Re: Changeset Extraction v7.6.1

On Mon, Feb 24, 2014 at 9:48 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-15 17:29:04 -0500, Robert Haas wrote:
>> On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
>> + /*
>> + * XXX: It's impolite to ignore our argument and keep decoding until the
>> + * current position.
>> + */
>>
>> Eh, what?
>
> So, the background here is that I was thinking of allowing to specify a
> limit for the number of returned rows. For the sql interface that sounds
> like a good idea. I am just not so sure anymore that allowing to specify
> a LSN as a limit is sufficient. Maybe simply allow to limit the number
> of changes and check everytime a transaction has been replayed?

The last idea there seems like pretty sound, but ...

> It's all trivial codewise, I am just wondering about the interface most
> users would want.

...I can't swear it meets this criterion.

>> + * We misuse the original meaning of SnapshotData's xip and
>> subxip fields
>> + * to make the more fitting for our needs.
>> [...]
>> + * XXX: Do we want extra fields instead of misusing existing
>> ones instead?
>>
>> If we're going to do this, then it surely needs to be documented in
>> snapshot.h. On the second question, you're not the first hacker to
>> want to abuse the meanings of the existing fields; SnapshotDirty
>> already does it. It's tempting to think we need a more principled
>> approach to this, like what we've done with Node i.e. typedef enum ...
>> SnapshotType; and then a separate struct definition for each kind, all
>> beginning with SnapshotType type.
>
> Hm, essentially that's what the ->satisfies pointer already is, right?

Sorta, yeah. But with nodes, you can change the whole struct
definition for each type.

> There's already documentation of the extra fields in snapbuild, but I
> understand you'd rather have them moved?

Yeah, I think it needs to be documented where SnapshotData is defined.
There may be reason to mention it again, or in more detail,
elsewhere. But there should be some mention of it there.

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