Обсуждение: [PATCH] updates to docs about HOT updates for BRIN

Greetings,

* Elizabeth Christensen (elizabeth.christensen@crunchydata.com) wrote:
> I have a small documentation patch to the HOT updates page
> <https://www.postgresql.org/docs/current/storage-hot.html>to add references
> to summary (BRIN) indexes not blocking HOT updates
> <https://www.postgresql.org/message-id/CAFp7QwpMRGcDAQumN7onN9HjrJ3u4X3ZRXdGFT0K5G2JWvnbWg@mail.gmail.com>,
> committed 19d8e2308b.

Sounds good to me, though the attached patch didn't want to apply, and
it isn't immediately clear to me why, but perhaps you could try saving
the patch from a different editor and re-sending it?

Thanks,

Stephen

> On Feb 26, 2024, at 4:21 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Elizabeth Christensen (elizabeth.christensen@crunchydata.com) wrote:
>> I have a small documentation patch to the HOT updates page
>> <https://www.postgresql.org/docs/current/storage-hot.html>to add references
>> to summary (BRIN) indexes not blocking HOT updates
>> <https://www.postgresql.org/message-id/CAFp7QwpMRGcDAQumN7onN9HjrJ3u4X3ZRXdGFT0K5G2JWvnbWg@mail.gmail.com>,
>> committed 19d8e2308b.
>
> Sounds good to me, though the attached patch didn't want to apply, and
> it isn't immediately clear to me why, but perhaps you could try saving
> the patch from a different editor and re-sending it?
>
> Thanks,
>
> Stephen

Thanks Stephen, attempt #2 here.

Elizabeth

Greetings,

* Elizabeth Christensen (elizabeth.christensen@crunchydata.com) wrote:
> > On Feb 26, 2024, at 4:21 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Elizabeth Christensen (elizabeth.christensen@crunchydata.com) wrote:
> >> I have a small documentation patch to the HOT updates page
> >> <https://www.postgresql.org/docs/current/storage-hot.html>to add references
> >> to summary (BRIN) indexes not blocking HOT updates
> >> <https://www.postgresql.org/message-id/CAFp7QwpMRGcDAQumN7onN9HjrJ3u4X3ZRXdGFT0K5G2JWvnbWg@mail.gmail.com>,
> >> committed 19d8e2308b.
> >
> > Sounds good to me, though the attached patch didn't want to apply, and
> > it isn't immediately clear to me why, but perhaps you could try saving
> > the patch from a different editor and re-sending it?
>
> Thanks Stephen, attempt #2 here.

Here's an updated patch which tries to improve on the wording a bit by
having it be a bit more consistent.  Would certainly welcome feedback on
it though, of course.  This is a tricky bit of language to write and
a complex optimization to explain.

Thanks!

Stephen

Hello,

On 2024-Feb-26, Stephen Frost wrote:

> Here's an updated patch which tries to improve on the wording a bit by
> having it be a bit more consistent.  Would certainly welcome feedback on
> it though, of course.  This is a tricky bit of language to write and
> a complex optimization to explain.

Should we try to explain what is a "summarizing" index is?  Right now
the only way to know is to look at the source code or attach a debugger
and see if IndexAmRoutine->amsummarizing is true.  Maybe we can just say
"currently the only in-core summarizing index is BRIN" somewhere in the
page.  (The patch's proposal to say "... such as BRIN" strikes me as too
vague.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
              https://postgr.es/m/200606261359.k5QDxE2p004593@auth-smtp.hol.gr

Greetings,

* Alvaro Herrera (alvherre@alvh.no-ip.org) wrote:
> On 2024-Feb-26, Stephen Frost wrote:
> > Here's an updated patch which tries to improve on the wording a bit by
> > having it be a bit more consistent.  Would certainly welcome feedback on
> > it though, of course.  This is a tricky bit of language to write and
> > a complex optimization to explain.
>
> Should we try to explain what is a "summarizing" index is?  Right now
> the only way to know is to look at the source code or attach a debugger
> and see if IndexAmRoutine->amsummarizing is true.  Maybe we can just say
> "currently the only in-core summarizing index is BRIN" somewhere in the
> page.  (The patch's proposal to say "... such as BRIN" strikes me as too
> vague.)

Not sure about explaining what one is, but I'd be fine adding that
language.  I was disappointed to see that there's no way to figure out
the value of amsummarizing for an access method other than looking at
the code.  Not as part of this specific patch, but I'd generally support
having a way to that information at the SQL level (or perhaps everything
from IndexAmRoutine?).

Attached is an updated patch which drops the 'such as' and adds a
sentence mentioning that BRIN is the only in-core summarizing index.

Thanks!

Stephen

On Tue, 2024-02-27 at 09:48 -0500, Stephen Frost wrote:
> Attached is an updated patch which drops the 'such as' and adds a
> sentence mentioning that BRIN is the only in-core summarizing index.

The original patch reads more clearly to me. In v4, summarizing (the
exception) feels like it's dominating the description.

Also, is it standard practice to backport this kind of doc update? I
ordinarily wouldn't be inclined to do so, but v4 seems targeted at 16
as well.

Attached my own suggested wording that hopefully addresses Stephen and
Alvaro's concerns. I agree that it's tricky to write so I took a more
minimalist approach:

 * I got rid of the "In summary" sentence because (a) it's confusing
now that we're talking about summarizing indexes; and (b) it's not
summarizing anything, it's just redundant.

 * I removed the mention partial or expression indexes. It's a bit
redundant and doesn't seem especially helpful in this context.

If this is agreeable I can commit it.

Regards,
    Jeff Davis

> On Mar 4, 2024, at 5:32 PM, Jeff Davis <pgsql@j-davis.com> wrote:

> Attached my own suggested wording that hopefully addresses Stephen and
> Alvaro's concerns. I agree that it's tricky to write so I took a more
> minimalist approach:
> 
> If this is agreeable I can commit it.
> 
> Regards,
>     Jeff Davis
> 
> <v5-0001-docs-Update-HOT-update-docs-for-19d8e2308b.patch>


Looks good to me. Thanks!

Elizabeth

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Tue, 2024-02-27 at 09:48 -0500, Stephen Frost wrote:
> > Attached is an updated patch which drops the 'such as' and adds a
> > sentence mentioning that BRIN is the only in-core summarizing index.
>
> The original patch reads more clearly to me. In v4, summarizing (the
> exception) feels like it's dominating the description.
>
> Also, is it standard practice to backport this kind of doc update? I
> ordinarily wouldn't be inclined to do so, but v4 seems targeted at 16
> as well.

I do think this change should be back-ported to when the change
happened, otherwise the documentation won't reflect what's in the
product for that version...

> Attached my own suggested wording that hopefully addresses Stephen and
> Alvaro's concerns. I agree that it's tricky to write so I took a more
> minimalist approach:

>  * I got rid of the "In summary" sentence because (a) it's confusing
> now that we're talking about summarizing indexes; and (b) it's not
> summarizing anything, it's just redundant.

>  * I removed the mention partial or expression indexes. It's a bit
> redundant and doesn't seem especially helpful in this context.

Just to point it out- the "In summary" did provide a bit of a summary,
before the 'partial or expression indexes' bit was removed.  That said,
I tend to still agree with these changes as I feel that users will
generally be able to infer that this applies to partial and expression
indexes without it having to be spelled out to them.

> If this is agreeable I can commit it.

Great, thanks!

Stephen

On Tue, 2024-03-05 at 09:53 -0600, Elizabeth Christensen wrote:
> Looks good to me. Thanks!

Thank you, committed.

Regards,
    Jeff Davis