On Tue, 31 Jan 2023 at 23:48, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2023-01-31 15:05:17 +0100, Matthias van de Meent wrote:
> > If TransactionIdRetreatSafely will be exposed outside procarray.c,
> > then I think the xid pointer should be replaced with normal
> > arguments/returns; both for parity with TransactionIdRetreatedBy
>
> That's why I named one version *Retreat the other Retreated :)
That part I noticed too :) I don't mind either way, I was just
concerned with exposing the function as a prototype, not as an inline
static.
> I think it'll make the code easier to read in the other places too, the
> variable names / function names in this space are uncomfortably long to
> fit into 78chars..., particularly when there's two references to the
> same variable in the same line.
I guess that's true, and once inlined there should indeed be no extra
runtime overhead.
> 78 chars
Didn't we use 80 columns/chars? How did you get to 78? Not that I
can't think of any ways, but none of them stand out to me as obviously
correct.
> > and to remove this memory store dependency in this hot code path.
>
> I doubt that matters much here and the places it's going to be used
> in.
I thought that this was executed while still in ProcArrayLock, but
instead we've released that lock already by the time we're trying to
adjust the horizons, so the 'hot code path' concern is mostly
relieved.
> And presumably the compiler will inline it anyway. I'd probably make
> it a static inline in the header too.
Yes, my concern was based on an extern prototype with private
implementation, as that does prohibit inlining and thus would have a
requirement to push the data to memory (probably only L1, but still
memory).
> What's making me hesitate about exposing it is that it's quite easy to
> get things wrong by using a wrong fxid or such.
I'm less concerned about that when the function is well-documented.
> > > + /*
> > > + * FIXME, doubtful this is the best fix.
> > > + *
> > > + * Can't represent the 32bit xid as a 64bit xid, as it's before fxid
> > > + * 0. Represent it as an xid from the future instead.
> > > + */
> > > + if (epoch == 0)
> > > + return FullTransactionIdFromEpochAndXid(0, xid);
> >
> > Shouldn't this be an error condition instead, as this XID should not
> > be able to appear?
>
> If you mean error in the sense of ERROR, no, I don't think so. That code
> tries hard to be able to check many tuples in a row. And if we were to
> error out here, we'd not able to do that. We should still report those
> tuples as corrupt, fwiw.
>
> The reason this path is hit is that a test intentionally corrupts some
> xids. So the path is reachable and we need to cope somehow.
I see.
Kind regards,
Matthias van de Meent