Re: Minimal logical decoding on standbys

Hi,

On 2023-01-06 10:52:06 +0100, Drouvot, Bertrand wrote:
> On 1/6/23 4:40 AM, Andres Freund wrote:
> > 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).

The problem I have with that is that I saw a lot of flakiness in the tests due
to the race condition. So introducing them in that order just doesn't make a
whole lot of sense to me. It's also something that can be committed
independently, I think.


> > 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.

The suggested path in earlier versions to avoid doing so was to make sure that
we pass down the Relation for the table into the necessary functions. Did you
explore that any further?


> 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.

I still dislike this approach. The code for cascading the change to the index
attributes is complicated. RelationIsAccessibleInLogicalDecoding() is getting
slower. We add unnecessary storage space to all pg_index rows.

Now I even wonder if this doesn't break the pg_index.indcheckxmin logic (which
I really dislike, but that's a separate discussion). I think updating pg_index
to set indisusercatalog might cause the index to be considered unusable, if
indcheckxmin = true. See

			/*
			 * If the index is valid, but cannot yet be used, ignore it; but
			 * mark the plan we are generating as transient. See
			 * src/backend/access/heap/README.HOT for discussion.
			 */
			if (index->indcheckxmin &&
				!TransactionIdPrecedes(HeapTupleHeaderGetXmin(indexRelation->rd_indextuple->t_data),
									   TransactionXmin))
			{
				root->glob->transientPlan = true;
				index_close(indexRelation, NoLock);
				continue;
			}


The reason we went with the indcheckxmin approach, instead of storing the xmin
after which an index is uable directly, was that that way we don't need
special logic around vacuum to reset the stored xid to prevent the index to
become unusable after xid wraparound. But these days we could just store a
64bit xid...

Greetings,

Andres Freund