On Sun, May 15, 2022 at 12:29 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Apr-20, Masahiko Sawada wrote:
>
> > > MyProc->statusFlags = (MyProc->statusFlags & ~PROC_XMIN_FLAGS) |
> > > (proc->statusFlags & PROC_XMIN_FLAGS);
> > >
> > > Perhaps the latter is more future-proof.
>
> > Copying only xmin-related flags in this way also makes sense to me and
> > there is no problem at least for now. A note would be that when we
> > introduce a new flag that needs to be copied in the future, we need to
> > make sure to add it to PROC_XMIN_FLAGS so it is copied. Otherwise a
> > similar issue we fixed by 0f0cfb494004befb0f6e could happen again.
>
> OK, done this way -- patch attached.
Thank you for updating the patch.
>
> Reading the comment I wrote about it, I wonder if flags
> PROC_AFFECTS_ALL_HORIZONS and PROC_IN_LOGICAL_DECODING should also be
> included. I think the only reason we don't care at this point is that
> walsenders (logical or otherwise) do not engage in snapshot copying.
> But if we were to implement usage of parallel workers sharing a common
> snapshot to do table sync in parallel, then it ISTM it would be
> important to copy at least the latter. Not sure there are any cases
> were we might care about the former.
Yeah, it seems to be inconsistent between the comment (and the new
name) and the flags actually included. I think we can include all
xmin-related flags to PROC_XMIN_FLAGS as the comment says. That way,
it would be useful also for other use cases, and I don't see any
downside for now.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/