On Fri, 5 Jun 2026 at 19:44, Corey Huinker <[email protected]> wrote:
> > > On Fri, Jun 5, 2026 at 7:02 AM Rafia Sabih <[email protected]> > wrote: > >> >> >> On Fri, 5 Jun 2026 at 07:43, Corey Huinker <[email protected]> >> wrote: >> >>> On Mon, Jun 1, 2026 at 7:55 AM Rafia Sabih <[email protected]> >>> wrote: >>> >>>> Hello hackers, >>>> >>>> I noticed that when configured batch_size is reduced because of libpq >>>> limit, in postgresGetForeignModifyBatchSize, there is no message about it >>>> that gets to the user. I am thinking that to keep the transparency it makes >>>> sense to add a debug message at least, as per the attached patch. >>>> >>>> What do you think? >>>> >>>> >>> I'm iffy on this one. >>> >>> The desire for instrumentation is generally good, and if a database was >>> having a problem where the tuned parameter wasn't having the desired effect >>> then it's understandable to want to know that. >>> >>> But I'm also not certain what different action someone would take if >>> they knew that this situation was happening. Do you anticipate some sort of >>> action on the user or DBA's part from this information beyond understanding >>> that's why you're not getting the full batch size? >>> >>> Hey Corey, >> >> Thanks for your opinion. As I said, it feels important from the >> transparency point to know when and why the batch_size changed for a case. >> >>> Thoughts on the code change itself: >>> >>> - The new variable (configured) should be declared in the new if/then >>> block, which is the narrowest scope possible. >>> >>> - The ereport() uses the old style with errmsg inside parens, and that's >>> no longer necessary. Existing ereport calls were left unchanged to avoid >>> code churn, but new ones should be in the simpler style. >>> >>> - This error is debuggy-enough that it might be an elog() rather than an >>> ereport() >>> >>> - I don't have good guidance on whether it should be DEBUG1 vs DEBUG2 or >>> a higher number. Curious if we have that guidance documented anywhere. >>> >>> - In the test case, choosing a batch_size > PQ_QUERY_PARAM_MAX_LIMIT >>> seems a bit artificial, in that it's a value that can't work for any query >>> with parameters, and someone might add sanity checks to batch_size in >>> option.c, which would in turn invalidate the test case. A test case with a >>> 2-column table should be able to get the same effect any batch_size greater >>> than half of 65535, yes? >>> >> >> I reworked the patch with your suggestions. >> -- >> Regards, >> Rafia Sabih >> CYBERTEC PostgreSQL International GmbH >> > > If you can, run pgindent on your changes before submitting a patch. > > The lack of a tab after "int" and the lack of a blank line after the var > declaration were easy to spot, but just in case I ran pgindent myself and > got: > > $ git diff > diff --git a/contrib/postgres_fdw/postgres_fdw.c > b/contrib/postgres_fdw/postgres_fdw.c > index 633451973fb..2e1bc47a737 100644 > --- a/contrib/postgres_fdw/postgres_fdw.c > +++ b/contrib/postgres_fdw/postgres_fdw.c > @@ -2305,12 +2305,13 @@ postgresGetForeignModifyBatchSize(ResultRelInfo > *resultRelInfo) > */ > if (fmstate && fmstate->p_nums > 0) > { > - int configured = batch_size; > + int configured = batch_size; > + > batch_size = Min(batch_size, PQ_QUERY_PARAM_MAX_LIMIT / > fmstate->p_nums); > if (batch_size < configured) > - elog(DEBUG1,"postgres_fdw: batch_size reduced from %d to %d " > - "to stay within the libpq %d-parameter limit", > - configured, batch_size, > PQ_QUERY_PARAM_MAX_LIMIT); > + elog(DEBUG1, "postgres_fdw: batch_size reduced from %d to %d " > + "to stay within the libpq %d-parameter limit", > + configured, batch_size, PQ_QUERY_PARAM_MAX_LIMIT); > } > > The elog may get changed back into an ereport. If it does, it'll come > under our wording guidelines scrutiny, but we'll worry about that later. > > Next, the test case still does "INSERT INTO ftable VALUES(1)", which > surprised me that it works even though ftable now has 2 columns. I think we > should change that to > > INSERT INTO ftable(x) VALUES(1): > > As this will remote the cognitive dissonance I experienced when looking at > the test case, and it has the bonus of demonstrating that the parameters > are generated in the SQL that postgres_fdw generates, not the SQL sent by > the user. > > As for the "should we even do this" question, the overhead seems pretty > small and it will only come up in corner cases, and even then only for > those who have signed up for the firehose of DEBUG1 messages, so I'm at > least a +0.5 on this now. > Thanks for your inputs. Reworked patch is attached. -- Regards, Rafia Sabih CYBERTEC PostgreSQL International GmbH
v3-0001-Emit-debug-message-for-batch_size-reduced.patch
Description: Binary data
