Re: logical changeset generation v6.8

On Wed, Dec 4, 2013 at 10:55 AM, Andres Freund <andres@2ndquadrant.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