On Wed, Nov 16, 2022 at 11:56 PM Andres Freund <andres@anarazel.de> wrote:
>
> On 2022-11-16 14:22:01 +0530, Amit Kapila wrote:
> > On Wed, Nov 16, 2022 at 7:30 AM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > On 2022-11-15 16:20:00 +0530, Amit Kapila wrote:
> > > > On Tue, Nov 15, 2022 at 8:08 AM Andres Freund <andres@anarazel.de> wrote:
> > > > > nor do we enforce in an obvious place that we
> > > > > don't already hold a snapshot.
> > > > >
> > > >
> > > > We have a check for (FirstXactSnapshot == NULL) in
> > > > RestoreTransactionSnapshot->SetTransactionSnapshot. Won't that be
> > > > sufficient?
> > >
> > > I don't think that'd e.g. catch a catalog snapshot being held, yet that'd
> > > still be bad. And I think checking in SetTransactionSnapshot() is too late,
> > > we've already overwritten MyProc->xmin by that point.
> > >
> >
> > So, shall we add the below Asserts in SnapBuildInitialSnapshot() after
> > we have the Assert for Assert(!FirstSnapshotSet)?
> > Assert(FirstXactSnapshot == NULL);
> > Assert(!HistoricSnapshotActive());
>
> I don't think that'd catch a catalog snapshot. But perhaps the better answer
> for the catalog snapshot is to just invalidate it explicitly. The user doesn't
> have control over the catalog snapshot being taken, and it's not too hard to
> imagine the walsender code triggering one somewhere.
>
> So maybe we should add something like:
>
> InvalidateCatalogSnapshot(); /* about to overwrite MyProc->xmin */
> if (HaveRegisteredOrActiveSnapshot())
> elog(ERROR, "cannot build an initial slot snapshot when snapshots exist")
> Assert(!HistoricSnapshotActive());
>
> I think we'd not need to assert FirstXactSnapshot == NULL or !FirstSnapshotSet
> with that, because those would show up in HaveRegisteredOrActiveSnapshot().
>
In the attached patch, I have incorporated this change and other
feedback. I think this should probably help us find the reason for
this problem when we see it in the future.
--
With Regards,
Amit Kapila.
