Re: postgres_fdw: Emit message when batch_size is reduced

On Fri, Jun 5, 2026 at 7:02 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
wrote:

>
>
> On Fri, 5 Jun 2026 at 07:43, Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
>
>> On Mon, Jun 1, 2026 at 7:55 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
>> 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.