Re: use less space in xl_xact_commit patch

On Wed, May 25, 2011 at 3:05 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, May 25, 2011 at 3:41 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Wed, May 25, 2011 at 2:43 PM, Leonardo Francalanci <m_lists(at)yahoo(dot)it> wrote:
>>>> Da: Simon Riggs <simon(at)2ndQuadrant(dot)com>
>>>> I can't find a clear  discussion of what you are trying to do, and how,
>>>> just a URL back to a  complex discussion on another topic.
>>>
>>>
>>> While trying to write a patch to allow changing an unlogged table into
>>> a logged one, I had to add another int field to xl_xact_commit.
>>> Robert Haas said:
>>>
>>>  "I have to admit I don't like this approach very much.  I can't see
>>> adding 4 bytes to every commit record for this feature."
>>>
>>>
>>> which is a correct remark.
>>>
>>> xl_xact_commit can contain some arrays (relation to drops,
>>> committed sub-trans, shared invalidation msgs). The length of
>>> these arrays is specified using 3 ints in the struct.
>>>
>>> So, to avoid adding more ints to the struct, I've been suggested to
>>> remove all the ints, and use   xl_xact_commit.xinfo to flag which
>>> arrays are, in fact, present.
>>>
>>> So the whole idea is:
>>>
>>> - remove nrels, nsubxacts and nmsgs from xl_xact_commit
>>> - use bits in xinfo to signal which arrays are present at the end
>>> of   xl_xact_commit
>>> - for each present array, add the length of the array (as int) at
>>> the end of    xl_xact_commit
>>> - add each present array after all the lengths
>>
>>
>> OK, thats clear. Thanks.
>>
>> That formatting sounds quite complex.
>>
>> I would propose we split this into 2 WAL records: xl_xact_commit and
>> xl_xact_commit_with_info
>>
>> xl_xact_commit doesn't have any flags, counts or arrays.
>>
>> xl_xact_commit_with_info always has all 3 counts, even if zero.
>> Arrays follow the main record
>>
>> I think it might also be possible to removed dbId and tsId from
>> xl_act_commit if we use that definition.
>
> Yes, that's correct. We can remove them from the normal commit record
> when nmsgs == 0.
>
> I think we can get away with these 2 definitions, but pls check. Using
> static definitions works better for me because we can see what they
> contain, rather than having the info flags imply that the record can
> contain any permutation of settings when that's not really possible.
>
>  {
>        TimestampTz xact_time;          /* time of commit */
>        int                     nsubxacts;              /* number of subtransaction XIDs */
>        /* ARRAY OF COMMITTED SUBTRANSACTION XIDs FOLLOWS */
>  } xl_xact_commit;
>
>  {
>        TimestampTz xact_time;          /* time of commit */
>        uint32          xinfo;                  /* info flags */
>        int                     nrels;                  /* number of RelFileNodes */
>        int                     nsubxacts;              /* number of subtransaction XIDs */
>        int                     nmsgs;          /* number of shared inval msgs */
>        Oid                     dbId;                   /* MyDatabaseId */
>        Oid                     tsId;                   /* MyDatabaseTableSpace */
>        /* Array of RelFileNode(s) to drop at commit */
>        RelFileNode xnodes[1];          /* VARIABLE LENGTH ARRAY */
>        /* ARRAY OF COMMITTED SUBTRANSACTION XIDs FOLLOWS */
>        /* ARRAY OF SHARED INVALIDATION MESSAGES FOLLOWS */
>  } xl_xact_commit_with_info;

Wow, that is identical to the conclusion that I came to about 60 seconds ago.

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