Re: postgres_fdw: Emit message when batch_size is reduced

On Fri, 5 Jun 2026 at 19:44, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:

>
>
> 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.
>

Thanks for your inputs. Reworked patch is attached.
--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH