public inbox for [email protected]  

help / color / mirror / Atom feed

From: Nikolay Shaplov <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: [email protected]
Cc: Michael Paquier <[email protected]>
Cc: Amit Langote <[email protected]>
Subject: Re: [PATCH] New [relation] option engine
Date: Wed, 18 May 2022 18:46:55 +0300
Message-ID: <1820455.Ed3y7NxtEv@thinkpad-pgpro> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

--nextPart1798714.Ef6jCgUoby
Content-Type: multipart/mixed; boundary="nextPart2827546.QrnxEpBih7"; protected-headers="v1"
Content-Transfer-Encoding: 7Bit
From: Nikolay Shaplov <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>, [email protected], Michael Paquier <[email protected]>, Amit Langote <[email protected]>
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: <[email protected]>
References: <[email protected]>

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"

view thread (20+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] New [relation] option engine
  In-Reply-To: <1820455.Ed3y7NxtEv@thinkpad-pgpro>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox