Feedback-ID: id4a34324:Fastmail
Date: Wed, 18 Jan 2023 18:46:17 -0800
From: Andres Freund <andres@anarazel.de>
To: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Thomas Munro <thomas.munro@gmail.com>,
	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>
Subject: Re: Minimal logical decoding on standbys
Message-ID: <20230119024617.jzsgpcrgjdc6ysge@awork3.anarazel.de>
References: <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>
 <490206c5-bcde-e558-628a-dc67dae4d439@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <490206c5-bcde-e558-628a-dc67dae4d439@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230119024617.jzsgpcrgjdc6ysge%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2023-01-18 11:24:19 +0100, Drouvot, Bertrand wrote:
> On 1/6/23 4:40 AM, Andres Freund wrote:
> > Hm, that's quite expensive. Perhaps worth adding a C helper that can do that
> > for us instead? This will likely also be needed in real applications after all.
> > 
> 
> Not sure I got it. What the C helper would be supposed to do?

Call LogStandbySnapshot().


> With a reload in place in my testing, now I notice that the catalog_xmin
> is updated on the primary physical slot after logical slots invalidation
> when reloading hot_standby_feedback from "off" to "on".
> 
> This is not the case after a re-start (aka catalog_xmin is NULL).
> 
> I think a re-start and reload should produce identical behavior on
> the primary physical slot. If so, I'm tempted to think that the catalog_xmin
> should be updated in case of a re-start too (even if all the logical slots are invalidated)
> because the slots are not dropped yet. What do you think?

I can't quite follow the steps leading up to the difference. Could you list
them in a bit more detail?


> > Can we do something cheaper than rewriting the entire database? Seems
> > rewriting a single table ought to be sufficient?
> > 
> 
> While implementing the test at the table level I discovered that It looks like there is no guarantee that say a "vacuum full pg_class;" would
> produce a conflict.

I assume that's mostly when there weren't any removal


> Indeed, from what I can see in my testing it could generate a XLOG_HEAP2_PRUNE with snapshotConflictHorizon to 0:
> 
> "rmgr: Heap2       len (rec/tot):    107/   107, tx:        848, lsn: 0/03B98B30, prev 0/03B98AF0, desc: PRUNE snapshotConflictHorizon 0"
> 
> 
> Having a snapshotConflictHorizon to zero leads to ResolveRecoveryConflictWithSnapshot() simply returning
> without any conflict handling.

That doesn't have to mean anything bad. Some row versions can be removed without
creating a conflict. See HeapTupleHeaderAdvanceConflictHorizon(), specifically

	 * Ignore tuples inserted by an aborted transaction or if the tuple was
	 * updated/deleted by the inserting transaction.


> It does look like that in the standby decoding case that's not the right behavior (and that the xid that generated the PRUNING should be used instead)
> , what do you think?

That'd not work, because that'll be typically newer than the catalog_xmin. So
we'd start invalidating things left and right, despite not needing to.


Did you see anything else around this making you suspicious?


> > > +##################################################
> > > +# Test standby promotion and logical decoding behavior
> > > +# after the standby gets promoted.
> > > +##################################################
> > > +
> > 
> > I think this also should test the streaming / walsender case.
> > 
> 
> Do you mean cascading standby?

I mean a logical walsender that starts on a standby and continues across
promotion of the standby.

Greetings,

Andres Freund