Hi, On 2014-02-25 13:47:49 -0500, Robert Haas wrote: > On Mon, Feb 24, 2014 at 6:16 PM, Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers