On Sat, 8 Aug 2026 at 21:29, solai v <[email protected]> wrote: > Hi all, > > On Fri, Aug 7, 2026 at 6:11 PM Nurlan Tulemisov > <[email protected]> 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 <[email protected]> > wrote: > >> > >> > >> > >> On Fri, 17 Jul 2026 at 18:39, Nurlan Tulemisov < > [email protected]> 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 < > [email protected]> wrote: > >>>> > >>>> > >>>> > >>>> On Sat, 11 Jul 2026 at 23:56, Nurlan Tulemisov < > [email protected]> 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 <[email protected] > >: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Mon, 22 Jun 2026 at 22:40, Corey Huinker < > [email protected]> wrote: > >>>>>>> > >>>>>>> On Fri, Jun 19, 2026 at 8:38 AM Rafia Sabih < > [email protected]> wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> On Tue, 16 Jun 2026 at 22:20, Corey Huinker < > [email protected]> wrote: > >>>>>>>>> > >>>>>>>>> On Wed, Jun 10, 2026 at 5:09 AM Rafia Sabih < > [email protected]> wrote: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On Tue, 9 Jun 2026 at 22:22, Corey Huinker < > [email protected]> 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
v8-0001-Emit-debug-message-for-batch_size-reduced.patch
Description: Binary data
