Re: Changeset Extraction v7.7

On Mon, Feb 24, 2014 at 6:16 PM, Andres Freund <andres(at)2ndquadrant(dot)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.

>> - 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?

>> + 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.

>> 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.

>> + /* ----
>> + * This is a bit tricky: We need to determine a safe xmin
>> horizon to start
>> + * decoding from, to avoid starting from a running xacts
>> record referring
>> + * to xids whose rows have been vacuumed or pruned
>> + * already. GetOldestSafeDecodingTransactionId() returns such
>> a value, but
>> + * without further interlock it's return value might
>> immediately be out of
>> + * date.
>> + *
>> + * So we have to acquire the ProcArrayLock to prevent computation of new
>> + * xmin horizons by other backends, get the safe decoding xid,
>> and inform
>> + * the slot machinery about the new limit. Once that's done the
>> + * ProcArrayLock can be be released as the slot machinery now is
>> + * protecting against vacuum.
>> + * ----
>> + */
>>
>> 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.

>> 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?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company