On Tue, 21 Jul 2026 at 20:47, 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. > Thanks for your time on this. Please mark it ready for committer then. > > 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 > -- Regards, Rafia Sabih CYBERTEC PostgreSQL International GmbH
