On Thu, Jul 18, 2024 at 09:57:23AM +0200, Daniel Gustafsson wrote: >> On 17 Jul 2024, at 23:32, Nathan Bossart <nathandboss...@gmail.com> wrote: >> On Wed, Jul 17, 2024 at 11:16:59PM +0200, Daniel Gustafsson wrote: > >>> +static void >>> +dispatch_query(const ClusterInfo *cluster, AsyncSlot *slot, >>> .... >>> + pg_free(query); >>> +} >>> >>> A minor point, perhaps fueled by me not having played around much with this >>> patchset. It seems a bit odd that dispatch_query is responsible for freeing >>> the query from the get_query callback. I would have expected the output >>> from >>> AsyncTaskGetQueryCB to be stored in AsyncTask and released by >>> async_task_free. >> >> I don't see any problem with doing it the way you suggest.
Actually, I do see a problem. If we do it this way, we'll have to store a string per database somewhere, which seems unnecessary. However, while looking into this, I noticed that only one get_query callback (get_db_subscription_count()) actually customizes the generated query using information in the provided DbInfo. AFAICT we can do this particular step without running a query in each database, as I mentioned elsewhere [0]. That should speed things up a bit and allow us to simplify the AsyncTask code. With that, if we are willing to assume that a given get_query callback will generate the same string for all databases (and I think we should), we can run the callback once and save the string in the step for dispatch_query() to use. This would look more like what you suggested in the quoted text. [0] https://postgr.es/m/ZprQJv_TxccN3tkr%40nathan -- nathan
