Re: postgres_fdw: Emit message when batch_size is reduced

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.

>
> Regards,
> Solai
>

--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH