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

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.

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