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