Message-ID: <5c5151a6-a1a3-6c38-7d68-543c9faa22f4@gmail.com>
Date: Fri, 6 Jan 2023 10:52:06 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: Minimal logical decoding on standbys
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>,
 Thomas Munro <thomas.munro@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
 Ibrar Ahmed <ibrar.ahmad@gmail.com>, Amit Khandekar
 <amitdkhan.pg@gmail.com>, fabriziomello@gmail.com,
 tushar <tushar.ahuja@enterprisedb.com>,
 Rahila Syed <rahila.syed@2ndquadrant.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <CA+TgmoaVcu_mbxbH=EccvKG6u8+MdQf9zx98uAL9zsStFwrYsQ@mail.gmail.com>
 <20221214173552.sshklqvwezeo52h3@alap3.anarazel.de>
 <CA+TgmoZ8-DXtDMMDGggFkSYyO8nmNdBKWjV6Ts-zYGR9p5sK5g@mail.gmail.com>
 <CA+TgmobmTvEUDVOBzARa9eD9XLqsvDW+v29dV+WvDKZPJAwNTQ@mail.gmail.com>
 <CA+TgmoaOsoQuXEo6gMq0GP8SyvCaS=ptYqdYc0uzDMoeShf_nw@mail.gmail.com>
 <CA+TgmoYTTsxP8y6uknZvCBNCRq+1FJ4zGbX8Px1TGW459fGsaQ@mail.gmail.com>
 <1f2ad9fd-53ff-74cc-a602-72d45423bcab@gmail.com>
 <a78de011-81cf-729c-e110-e6d636fdca4d@gmail.com>
 <05c0c4f3-82eb-bc6d-c876-5a265c05927b@gmail.com>
 <CA+TgmoY8LAaTwAhS+YNQNrM+3K8eqxGvPT3gbJaHER86XShvFw@mail.gmail.com>
 <20230106034036.2m4qnn7ep7b5ipet@awork3.anarazel.de>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <20230106034036.2m4qnn7ep7b5ipet@awork3.anarazel.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/5c5151a6-a1a3-6c38-7d68-543c9faa22f4%40gmail.com>
Precedence: bulk

Hi,

On 1/6/23 4:40 AM, Andres Freund wrote:
> Hi,
> 
> Thomas, there's one point at the bottom wrt ConditionVariables that'd be
> interesting for you to comment on.
> 
> 
> On 2023-01-05 16:15:39 -0500, Robert Haas wrote:
>> On Tue, Jan 3, 2023 at 2:42 AM Drouvot, Bertrand
>> <bertranddrouvot.pg@gmail.com> wrote:
>>> Please find attached v36, tiny rebase due to 1de58df4fe.
>>
>> 0001 looks committable to me now, though we probably shouldn't do that
>> unless we're pretty confident about shipping enough of the rest of
>> this to accomplish something useful.
> 

Thanks for your precious help reaching this state!

> Cool!
> 
> ISTM that the ordering of patches isn't quite right later on. ISTM that it
> doesn't make sense to introduce working logic decoding without first fixing
> WalSndWaitForWal() (i.e. patch 0006). What made you order the patches that
> way?
> 

Idea was to ease the review: 0001 to 0005 to introduce the feature and 0006 to deal
with this race condition.

I thought it would be easier to review that way (given the complexity of "just" adding the
feature itself).

> 0001:
>> 4. We can't rely on the standby's relcache entries for this purpose in
>> any way, because the WAL record that causes the problem might be
>> replayed before the standby even reaches consistency.
> 
> The startup process can't access catalog contents in the first place, so the
> consistency issue is secondary.
> 

Thanks for pointing out, I'll update the commit message.

> 
> ISTM that the commit message omits a fairly significant portion of the change:
> The introduction of indisusercatalog / the reason for its introduction.

Agree, will do (or create a dedicated path as you are suggesting below).

> 
> Why is indisusercatalog stored as "full" column, whereas we store the fact of
> table being used as a catalog table in a reloption? I'm not adverse to moving
> to a full column, but then I think we should do the same for tables.
> 
> Earlier version of the patches IIRC sourced the "catalogness" from the
> relation. What lead you to changing that? I'm not saying it's wrong, just not
> sure it's right either.

That's right it's started retrieving this information from the relation.

Then, Robert made a comment in [1] saying it's not safe to call
table_open() while holding a buffer lock.

Then, I worked on other options and submitted the current one.

While reviewing 0001, Robert's also thought of it (see [2])) and finished with:

"
So while I do not really like the approach of storing the same
property in different ways for tables and for indexes, it's also not
really obvious to me how to do better.
"

That's also my thought.

> 
> It'd be good to introduce cross-checks that indisusercatalog is set
> correctly. RelationGetIndexList() seems like a good candidate.
> 

Good point, will look at it.

> I'd probably split the introduction of indisusercatalog into a separate patch.

You mean, completely outside of this patch series or a sub-patch in this series?
If the former, I'm not sure it would make sense outside of the current context.

> 
> Why was HEAP_DEFAULT_USER_CATALOG_TABLE introduced in this patch?
> 
> 

To help in case of reset on the table (ensure the default gets also propagated to the indexes).

> I wonder if we instead should compute a relation's "catalogness" in the
> relcache. That'd would have the advantage of making
> RelationIsUsedAsCatalogTable() cheaper and working for all kinds of
> relations.
> 

Any idea on where and how you'd do that? (that's one option I explored in vain before
submitting the current proposal).

It does look like that's also an option explored by Robert in [2]:

"
Yet a third way is to have the index fetch the flag from
the associated table, perhaps when the relcache entry is built. But I
see no existing precedent for that in RelationInitIndexAccessInfo,
which I think is where it would be if we had it -- and that makes me
suspect that there might be good reasons why this isn't actually safe.
"

> 
> VISIBILITYMAP_ON_CATALOG_ACCESSIBLE_IN_LOGICAL_DECODING is a very long
> identifier. Given that the field in the xlog records is just named
> isCatalogRel, any reason to not just name it correspondingly?
> 

Agree, VISIBILITYMAP_IS_CATALOG_REL maybe?

I'll look at the other comments too and work on/reply later on.

[1]: https://www.postgresql.org/message-id/CA%2BTgmobgOLH-JpBoBSdu4i%2BsjRdgwmDEZGECkmowXqQgQL6WhQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CA%2BTgmoY0df9X%2B5ENg8P0BGj0odhM45sdQ7kB4JMo4NKaoFy-Vg%40mail.gmail.com

Thanks for your help,
Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com