Re: Minimal logical decoding on standbys

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