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
v7-0001-Emit-debug-message-for-batch_size-reduced.patch
Description: Binary data
