Re: [PATCH] New [relation] option engine

From: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Michael Paquier <michael(at)paquier(dot)xyz>, Amit Langote <amitlangote09(at)gmail(dot)com>
Subject: Re: [PATCH] New [relation] option engine
Date: 2022-05-18 15:46:55
Message-ID: 1820455.Ed3y7NxtEv@thinkpad-pgpro
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

В письме от среда, 18 мая 2022 г. 11:10:08 MSK пользователь Alvaro Herrera
> forbid_realloc is only tested in an assert. There needs to be an "if"
> test for it somewhere (suppose some extension author uses this API and
> only runs it in assert-disabled environment; they'll never know they
> made a mistake). But do we really need this option? Why do we need a
> hardcoded limit in the number of options?

General idea was that it is better to allocate as many option_spec items as we
are going to use. It is optional, so if you do not want to allocate exact
number of options, then realloc will be used, when we adding one more item,
and all allocated items are used.

But when you explicitly specify number of items, it is better not to forget to
++ it when you add extra option in the code. That was the purpose of
forbid_realloc: to remind. It worked well for, while working with the patch
several options were added in the upstream, and this assert reminded me that I
should also allocate extra item.

If it is run in production without asserts, it is also no big deal, we will
just have another realloc.

But you are right, variable name forbid_realloc is misleading. It does not
really forbid anything, so I changed it to assert_on_realloc, so the name
tells what this flag really do.

> In allocateOptionsSpecSet there's a new error message with a typo
> "grater" which should be "greater". But I think the message is
> confusingly worded. Maybe a better wording is "the value of parameter
> XXX may not be greater than YYY".

This error message is really from bloom index. And here I was not as careful
and watchful as I should, because this error message is from the check that
was not there in original code. And this patch should not change behaviour at
all. So I removed this check completely, and will submit it later.

My original patch has a bunch of changes like that. I've removed them all, but
missed one in the contrib... :-(

Thank you for pointing to it.
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM:

Attachment Content-Type Size
new_options_take_two_v03a.patch text/x-patch 199.6 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2022-05-18 16:08:27 Re: Valgrind mem-check for postgres extension
Previous Message Jan Wieck 2022-05-18 15:41:54 Re: Limiting memory allocation