From: Nikolay Shaplov <dhyan@nataraj.su>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
 Michael Paquier <michael@paquier.xyz>, Amit Langote <amitlangote09@gmail.com>
Subject: Re: [PATCH] New [relation] option engine
Date: Wed, 18 May 2022 18:46:55 +0300
Message-ID: <1820455.Ed3y7NxtEv@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <202205180810.fx2gct2x47gd@alvherre.pgsql>
References: <202205180810.fx2gct2x47gd@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart1798714.Ef6jCgUoby";
 micalg="pgp-sha512"; protocol="application/pgp-signature"
Archived-At: 
 <https://www.postgresql.org/message-id/1820455.Ed3y7NxtEv%40thinkpad-pgpro>
Precedence: bulk

--nextPart1798714.Ef6jCgUoby
Content-Type: multipart/mixed; boundary="nextPart2827546.QrnxEpBih7"; protected-headers="v1"
Content-Transfer-Encoding: 7Bit
From: Nikolay Shaplov <dhyan@nataraj.su>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>, Amit Langote <amitlangote09@gmail.com>
Subject: Re: [PATCH] New [relation] option engine
Date: Wed, 18 May 2022 18:46:55 +0300
Message-ID: <1820455.Ed3y7NxtEv@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <202205180810.fx2gct2x47gd@alvherre.pgsql>
References: <202205180810.fx2gct2x47gd@alvherre.pgsql>

This is a multi-part message in MIME format.

--nextPart2827546.QrnxEpBih7
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D1=81=D1=80=D0=B5=
=D0=B4=D0=B0, 18 =D0=BC=D0=B0=D1=8F 2022 =D0=B3. 11:10:08 MSK =D0=BF=D0=BE=
=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C Alvaro Herrera=
=20
=D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:
> 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=20
are going to use. It is optional, so if you do not want to allocate exact=20
number of options, then realloc will be used, when we adding one more item,=
=20
and all allocated items are used.

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

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

But you are right, variable name forbid_realloc is misleading. It does not=
=20
really forbid anything, so I changed it to assert_on_realloc, so the name=20
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 carefu=
l=20
and watchful as I should, because this error message is from the check that=
=20
was not there in original code. And this patch should not change behaviour =
at=20
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=20
missed one in the contrib... :-(

Thank you for pointing to it.=20
=2D-=20
Nikolay Shaplov aka Nataraj
=46uzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

--nextPart2827546.QrnxEpBih7
Content-Disposition: attachment; filename="new_options_take_two_v03a.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="UTF-8"; name="new_options_take_two_v03a.patch"