Horton

Re: [PATCH] New [relation] option engine

Nikolay Shaplov <dhyan@nataraj.su>

View thread
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>
Date: 2022-05-18T15:46:55Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Tighten test_predtest's input checks, and improve error messages.

  2. Avoid reference to nonexistent array element in ExecInitAgg().

  3. Clean up newlines following left parentheses

Attachments

В письме от среда, 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: @dhyan:nataraj.su