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 > -- Regards, Nurlan
