On 12/12/2024 22:26, Heikki Linnakangas wrote:
> On 12/12/2024 21:57, Andres Freund wrote:
>> Perhaps we should have some assertions ensuring TransactionXmin has a
>> valid
>> value in some places?
>
> +1, wouldn't hurt.
I didn't, after all, as I couldn't find a good place where to put that.
>>> There are a two other places where we set MyProc->xmin without updating
>>> TransactionXmin:
>>>
>>> 1. In ProcessStandbyHSFeedbackMessage(). AFAICS that's OK because
>>> walsender
>>> doesn't use TransactionXmin for anything.
>>
>> It's worth noting that a walsender connection can do normal query
>> processing
>> these days if connected to a database....
Hmm; they will use regular snapshots in that case, which will set
TransactionXmin. But yes, good point nevertheless.
>>> 2. In SnapBuildInitialSnapshot(). I suppose that's also OK because the
>>> TransactionXmin isn't used. I don't quite remember when that function
>>> might
>>> be called though.
>>
>> It's used to export a snapshot after creating a logical slot. The
>> snapshot can
>> be used to sync the existing data, before starting to apply future
>> changes.
>>
>> I don't see a need to modify TransactionXmin here, it's not a normal
>> snapshot,
>> and as a comment in the function says, we rely on the slot's xmin to
>> protect
>> against relevant rows being removed.
>>
>>> In any case, I propose that we set TransactionXmin in all of those
>>> cases as
>>> well, so that TransactionXmin is always the equal to MyProc->xmin. Maybe
>>> even rename it to MyProcXmin to make that more clear.
>>
>> I'm not sure it's really right to do that. If we don't hold a
>> snapshot, what
>> makes sure that we then call SnapshotResetXmin() or such to reset
>> TransactionXmin?
>
> I don't understand. What I was meant is that we make it a strict rule
> that whenever you change MyProc->xmin, you always update TransactionXmin
> too, so that TransactionXmin == MyProc->xmin is always true.
I committed a minimal fix in SnapshotResetXmin() only, in all stable
branches. I still think it might be a good idea to make it a strict
invariant that TransactionXmin == MyProc->xmin is always true. But it's
not wrong as it is, if you think that TransactionXmin is the xmin of the
oldest active snapshot in the system, so that it might not be set if
MyProc->xmin is held back by something else than a snapshot, like in the
walsender processes.
--
Heikki Linnakangas
Neon (https://neon.tech)
