Re: postgres_fdw: Emit message when batch_size is reduced

Hi,

On Mon, Aug 10, 2026 at 3:44 PM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
>
>
>
> On Sat, 8 Aug 2026 at 21:29, solai v <solai(dot)cdac(at)gmail(dot)com> wrote:
>>
>> Hi all,
>>
>> On Fri, Aug 7, 2026 at 6:11 PM Nurlan Tulemisov
>> <nurlan(dot)tulemisov(at)gmail(dot)com> wrote:
>> >
>> > Hi Rafia,
>> >
>> > I reviewed v7. I have no further concerns with the current version, and the patch looks ready to me.
>> >
>> > On Mon, Jul 20, 2026 at 12:16 PM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
>> >>
>> >>
>> >>
>> >> On Fri, 17 Jul 2026 at 18:39, Nurlan Tulemisov <nurlan(dot)tulemisov(at)gmail(dot)com> wrote:
>> >>>
>> >>> Hi Rafia,
>> >>>
>> >>>> Wouldn't that give duplicate warnings with the changes already in the patch...or you are suggesting to have it only in postgres_fdw_validator() ?
>> >>>
>> >>> Yes, after thinking about it some more, I think it would be better to emit the WARNING only from postgres_fdw_validator() when the option is SET (CREATE/ALTER), while still allowing values greater than PQ_QUERY_PARAM_MAX_LIMIT to be stored.
>> >>>
>> >>> This also seems consistent with Corey's earlier suggestion:
>> >>>>
>> >>>> I'm saying that we may want a separate check when the batch_size option is
>> >>>> SET (via CREATE/ALTER TABLE/SERVER) to test the value against
>> >>>> PQ_QUERY_PARAM_MAX_LIMIT
>> >>>> and issue a NOTICE/WARNING that the chosen value will always have to be set
>> >>>> at or below $PQ_QUERY_PARAM_MAX_LIMIT - but still let them store the values
>> >>>> as-is.
>> >>>
>> >>>
>> >>> The WARNING currently emitted during INSERT seems potentially noisy, since it may be repeated every time the foreign table is used.
>> >>>
>> >>> Nit: since this table is used to verify that no warning is emitted:
>> >>>>
>> >>>> +-- Verify that no WARNING is emitted when batch_size is within the
>> >>>> +-- libpq 65535-parameter limit.
>> >>>> +CREATE TABLE batch_warn_table ( x int );
>> >>>
>> >>> Would batch_no_warn_table be a clearer name? The current name may be slightly confusing in this context.
>> >>>
>> >> Thank you for the review, I have made both the changes in the attached version.
>> >>>
>> >>> On Thu, Jul 16, 2026 at 10:51 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
>> >>>>
>> >>>>
>> >>>>
>> >>>> On Sat, 11 Jul 2026 at 23:56, Nurlan Tulemisov <nurlan(dot)tulemisov(at)gmail(dot)com> wrote:
>> >>>>>
>> >>>>> Hi,
>> >>>>>
>> >>>>> I reviewed v5, applied it to the current master, and ran the postgres_fdw regression tests. The patch applies cleanly and the tests pass.
>> >>>>
>> >>>> Thank you for the review.
>> >>>>>
>> >>>>>
>> >>>>> Would it also make sense to emit a NOTICE or WARNING from postgres_fdw_validator() in option.c when batch_size is set to a value greater than PQ_QUERY_PARAM_MAX_LIMIT?
>> >>>>>
>> >>>> Wouldn't that give duplicate warnings with the changes already in the patch...or you are suggesting to have it only in postgres_fdw_validator() ?
>> >>>>>
>> >>>>> That would notify the user immediately during CREATE or ALTER, while still accepting the value and preserving the existing behavior.
>> >>>>>
>> >>>>> The existing tests cover cases where the messages are emitted, but I think it may also be useful to cover the exact boundaries and the cases where no message should be emitted.
>> >>>>>
>> >>>>> For a two-parameter foreign insert, batch_size = 32767 should not produce a DEBUG1 message, while batch_size = 32768 should be reduced to 32767. Similarly, for a single parameter, batch_size = 65535 should not produce a warning, while 65536 should.
>> >>>>
>> >>>> Done.
>> >>>> Please find the updated patch attached.
>> >>>>>
>> >>>>> Regards,
>> >>>>> Nurlan
>> >>>>>
>> >>>>>
>> >>>>> сб, 11 июл. 2026 г. в 21:13, Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>:
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> On Mon, 22 Jun 2026 at 22:40, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>> >>>>>>>
>> >>>>>>> On Fri, Jun 19, 2026 at 8:38 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> On Tue, 16 Jun 2026 at 22:20, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>> >>>>>>>>>
>> >>>>>>>>> On Wed, Jun 10, 2026 at 5:09 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>> On Tue, 9 Jun 2026 at 22:22, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>> >>>>>>>>>>>>
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> Thanks for your inputs. Reworked patch is attached.
>> >>>>>>>>>>>> --
>> >>>>>>>>>>>> Regards,
>> >>>>>>>>>>>> Rafia Sabih
>> >>>>>>>>>>>> CYBERTEC PostgreSQL International GmbH
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> You've addressed all my concerns, aside from the desire for the check on the set/update of the value. Do you have a commitfest entry? I didn't find one.
>> >>>>>>>>>>
>> >>>>>>>>>> There is commitfest entry now --> https://commitfest.postgresql.org/patch/6873/
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> I've added myself as a reviewer. Did you want to try adding the check at time of the option being set? If not, I can make an attempt at that.
>> >>>>>>>>
>> >>>>>>>> Please find the attached file for the patch with the warning message at the time of batch_size option addition. Looking forward to your inputs.
>> >>>>>>>>
>> >>>>>>>> --
>> >>>>>>>> Regards,
>> >>>>>>>> Rafia Sabih
>> >>>>>>>> CYBERTEC PostgreSQL International GmbH
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> Applies clean, passes.
>> >>>>>>>
>> >>>>>>> I think we need to tweak the elog() below:
>> >>>>>>>
>> >>>>>>> + if (batch_size > PQ_QUERY_PARAM_MAX_LIMIT)
>> >>>>>>> + elog(WARNING, "postgres_fdw: batch_size %d is at or above the libpq "
>> >>>>>>> + "%d-parameter limit; the effective per-batch ceiling is "
>> >>>>>>> + "limit / number_of_columns and may be lower",
>> >>>>>>> + batch_size, PQ_QUERY_PARAM_MAX_LIMIT);
>> >>>>>>>
>> >>>>>>> I think this should be an ereport() because it's the sort of error we'd want the caller to see, and that means we need the message to conform the guidelines at https://www.postgresql.org/docs/current/error-style-guide.html, and I'm going to suggest this as a starting point:
>> >>>>>>>
>> >>>>>>> ereport(WARNING,
>> >>>>>>> errmsg("%s of %d exceeds protocol limit of %d", "batch_size", batch_size, PQ_QUERY_PARAM_MAX_LIMIT),
>> >>>>>>> errdetail("The %s for a query will be reduced to protocol limit divided by the number of columns in the query.", "batch_size"));
>> >>>>>>>
>> >>>>>> Done.
>> >>>>>>>
>> >>>>>>> I'd like to hear other people's opinions on what the proper conforming error message would be.
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> --
>> >>>>>> Regards,
>> >>>>>> Rafia Sabih
>> >>>>>> CYBERTEC PostgreSQL International GmbH
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> --
>> >>>>> Regards,
>> >>>>> Nurlan
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Regards,
>> >>>> Rafia Sabih
>> >>>> CYBERTEC PostgreSQL International GmbH
>> >>
>> >>
>> >>
>> >> --
>> >> Regards,
>> >> Rafia Sabih
>> >> CYBERTEC PostgreSQL International GmbH
>> >
>> >
>>
>> I reviewed and tested the patch. The change in
>> postgresGetForeignModifyBatchSize() looks correct. The effective
>> batch_size is now limited based on the number of parameters being
>> sent, using PQ_QUERY_PARAM_MAX_LIMIT / fmstate->p_nums. I tested the
>> behavior with different numbers of columns and boundary values. The
>> results were as expected:
>>
>> 1. With batch_size = 65536 and one column, a warning is emitted when
>> the foreign table is created, and the batch size is reduced to 65535
>> during execution.
>> 2. With two columns, 33000 is reduced to 32767.
>> 3. 32768 is reduced to 32767, while 32767 is accepted without reduction.
>> 4. With five columns, 13108 is reduced to 13107, while 13107 is accepted.
>> 5. Invalid values such as 0 and -1 are still rejected correctly.
>>
>> I also verified the inserted data on the remote server and the row
>> counts and values were correct. The existing postgres_fdw tests also
>> pass - make -C contrib/postgres_fdw check and git diff --check also
>> reports no issues. I noticed one issue in the regression test that I
>> think should be addressed. The test is intended to verify the
>> two-column parameter limit, but it currently has:
>>
>> CREATE TABLE batch_table (x int, y int);
>> CREATE FOREIGN TABLE ftable (x int, y int)
>> SERVER loopback
>> OPTIONS (table_name 'batch_table', batch_size '33000');
>> INSERT INTO ftable(x) VALUES (1);
>>
>> Since the INSERT only specifies x, only one parameter is actually
>> sent. Therefore, this doesn't exercise the p_nums = 2 case, even
>> though the expected output shows - DEBUG: batch_size reduced from
>> 33000 to 32767. I think the INSERT should use both columns, for
>> example: INSERT INTO ftable(x, y) VALUES (1, 1); The same applies to
>> the later 32767 boundary test.
>> Apart from this test issue, the implementation and the behavior I
>> tested look good. Looking forward to more feedback on this.
>>
> Thank you Solai for your time and review on this.
> Please find the attached patch for the updated version.
>>

Thanks for considering my suggestion and sending the updated patch. I
reviewed it and the changes look good to me. I tested the batch size
handling with different numbers of columns and verified that the batch
size is reduced correctly when the number of parameters would exceed
the libpq limit. In particular, I checked the boundary cases with two
columns, where 32767 is allowed and 32768 is reduced to 32767. I also
verified that the appropriate DEBUG1 message is emitted when the batch
size is reduced, and that the existing warning is shown when the
configured batch size is greater than the protocol limit. Invalid
values such as 0 and -1 continue to be rejected as expected. The
updated regression test also looks better to me since it now uses both
columns in the INSERT, so it actually exercises the parameter-limit
calculation for multiple columns. I ran the postgres_fdw regression
and isolation tests as well and both tests passed successfully. I also
verified the inserted data on the remote server and did not observe
any issues.
Overall, the changes look good to me, and I don't have any further comments.

Regards,
Solai