Re: Changeset Extraction v7.7 - Mailing list pgsql-hackers

Hi,

On 2014-02-25 13:47:49 -0500, Robert Haas wrote:
> On Mon, Feb 24, 2014 at 6:16 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > I actually thought they'd be too ugly to live and we'd remove them
> > pre-commit.
> 
> Might be getting to be about that time, then.

I want to leave them in until the slot semantics aren't going to change
anymore, they are pretty useful for testing that. But I'll separate them out
into a separate commit again.

> >> -               if (nrels > 0 || nmsgs > 0 || RelcacheInitFileInval ||
> >> forceSyncCommit)
> >> +               if (nrels > 0 || nmsgs > 0 || RelcacheInitFileInval ||
> >> forceSyncCommit ||
> >> +                       XLogLogicalInfoActive())
> >>
> >> Mmph.  Is this really necessary?  If so, why?  The comments could elucidate.
> >
> > We could get rid of it by (optionally) adding information about the
> > database oid to compact commit, but that'd increase the size of the
> > record.
> 
> So why do we need the database OID?

To ignore commits from other databases. Since we don't decode changes
from other databases, it's really confusing (and pointless overhead) to
see transactions from there.

> >> +               bool fail_softly = slot->data.persistency == RS_EPHEMERAL;
> >>
> >> This should be contingent on whether we're being called in the error
> >> pathway, not the slot type.  I think you should pass a bool.
> >
> > Why? I had it that way at first, but for persistent slots this won't be
> > called in error pathways as we won't drop there.
> 
> I was thinking more the reverse - that a non-persistent slot might be
> dropped in a non-error pathway.

Well, currently EPHEMERAL slots are documented to be dropped at release
since that's what changeset extraction (and possibly basebackup and
receivexlog) need afaics. You'd prefer DROP_ON_ERROR semantics?

> >>  It seems to be indicating, roughly, whether the relation should
> >> participate in RecentGlobalXmin or RecentGlobalDataXmin.  But is there
> >> any point at all of separating those when !XLogLogicalInfoActive()?
> >> The test expands to:
> >>
> >> IsSystemRelation() || (XLogLogicalInfoActive() &&
> >> RelationNeedsWAL(relation) && (IsCatalogRelation(relation) ||
> >> RelationIsUsedAsCatalogTable(relation)))
> >>
> >> So basically this is all tables created in pg_catalog during initdb
> >> plus all TOAST tables in the system.  If wal_level=logical, then we
> >> also include tables marked with the reloption user_catalog_table=true,
> >> unless they're unlogged.  This all seems a bit complex.  Why not this:
> >>
> >> IsSystemRelation() || || RelationIsUsedAsCatalogTable(relation)
> >
> > Because that'd possibly retain too much when !XLogLogicalInfoActive(),
> > there's no need to look for RelationIsUsedAsCatalogTable() for those. We
> > could decide not to care?
> 
> But when !XLogLogicalInfoActive() I think we could just make this
> always false, right?  I mean, if PROC_IN_LOGICAL_DECODING is never
> going to be set,  the values are always going to be the same anyway.
> I think.

It seems confusing and bug-prone to use the wrong horizon variable just
because right now they'd be the same if wal_level < logical.

> >> I can't claim to be very excited about this.
> >
> > Because of the already_locked parameters, or any wider concerns?
> 
> Passing down already_locked through several layers is kind of ugly,
> but also, holding ProcArrayLock more is sad.  That is not a
> lightly-contended lock.

Absolutely true, but this is very far from a operation that will be
frequent enough to matter. Creating a slot so frequently that a lock on
the procarray hold while iterating the slot array matters, will be
painful long before the contention on that is the problem.

> >> I'm assuming you've
> >> spent a lot of time thinking about ways to avoid this and utterly
> >> failed to come up with any reasonable alternative, but let me take a
> >> shot.  Suppose we take ProcArrayLock in exclusive mode and compute the
> >> oldest running XID, install it as our xmin, and then release
> >> ProcArrayLock.  At that point, nobody else can compute an oldest-xmin
> >> value that precedes that value, so we can take our time installing
> >> that value as the slot's xmin, without needing to hold a lock
> >> meanwhile.
> >
> > I actually had it that way for a while, but what if the backend already
> > has a xmin set? Then we need to reason about whether the xmin is newer,
> > restore it afterwards and such. That doesn't seem nice.
> 
> It's not too far removed from the problem snapmgr.c is already
> designed to solve, though, is it?

Hm, I don't immediately see how it would fit in there. PgXact->xmin is
set by procarray.c, all snapmgr does is reset it. And there's no logic
about resetting it back to higher values and such.

I'll ponder on getting rid of this, but I am not of too high hopes.

Thanks,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services