Re: logical changeset generation v6.8

On Wed, Dec 4, 2013 at 10:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-12-03 15:19:26 -0500, Robert Haas wrote:
>> Yeah, you're right. I think the current logic will terminate when all
>> flags are set to false or all attribute numbers have been checked, but
>> it doesn't know that if HOT's been disproven then we needn't consider
>> further HOT columns. I think the way to fix that is to tweak this
>> part:
>>
>> + if (next_hot_attnum > FirstLowInvalidHeapAttributeNumber)
>> check_now = next_hot_attnum;
>> + else if (next_key_attnum > FirstLowInvalidHeapAttributeNumber)
>> + check_now = next_key_attnum;
>> + else if (next_id_attnum > FirstLowInvalidHeapAttributeNumber)
>> + check_now = next_id_attnum;
>> else
>> + break;
>>
>> What I think we ought to do there is change each of those criteria to
>> say if (hot_result && next_hot_attnum >
>> FirstLowInvalidHeapAttributeNumber) and similarly for the other two.
>> That way we consider each set a valid source of attribute numbers only
>> until the result flag for that set flips false.
>
> That seems to work well, yes.
>
> Updated & rebased series attached.
>
> * Rebased since the former patch 01 has been applied
> * Lots of smaller changes in the wal_level=logical patch
> * Use Robert's version of wal_level=logical, with the above fixes
> * Use only macros for RelationIsAccessibleInLogicalDecoding/LogicallyLogged
> * Moved a mit more logic into ExtractReplicaIdentity
> * some comment copy-editing
> * Bug noted by Euler fixed, testcase added
> * Some copy editing in later patches, nothing significant.
>
> I've primarily sent this, because I don't know of further required
> changes in 0001-0003. I am trying reviewing the other patches in detail
> atm.

Committed #1 (again). Regarding this:

+ /* XXX: we could also do this unconditionally, the space is used anyway
+ if (copy_oid)
+ HeapTupleSetOid(key_tuple, HeapTupleGetOid(tp));

I would like to put in a big +1 for doing that unconditionally. I
didn't make that change before committing, but I think it'd be a very
good idea.

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