Keeping pg_recvlogical's "feTimestamp" separate from TimestampTz

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Cc: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
Subject: Keeping pg_recvlogical's "feTimestamp" separate from TimestampTz
Date: 2017-02-17 18:15:38
Message-ID: 32046.1487355338@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Thomas Munro pointed out that commit 7c030783a broke things on
--disable-integer-datetimes builds, because somebody cleverly used
TimestampTz to declare timestamp variables, no doubt not having
read the comment (which doesn't even appear in the same file :-()
that

* ... The replication protocol always uses integer timestamps,
* regardless of the server setting.

I am not sure that it was really a good design to pretend that the
replication protocol is independent of --disable-integer-datetimes
when the underlying WAL stream most certainly isn't.

However, if we keep it like this, I think we need to expend significantly
more effort on making sure that "feTimestamps" aren't confused with
TimestampTzs. It's only the sheer happenstance that a couple of new
functions were declared with "TimestampTz *" not "TimestampTz" arguments
that we found this mechanically at all --- otherwise, the compiler doesn't
know any better than to naively cast from int64 to double or vice versa
when asked to.

Moreover, I think there is absolutely nothing stopping somebody from
trying to compare a replication-protocol timestamp to a TimestampTz
taken out of the WAL stream, which will give completely wrong answers
for float timestamps, and which no compiler on earth will warn about.
Even if there are no such occurrences today, does anyone really want
to bet that somebody won't submit a patch tomorrow that subtly depends
on replication-protocol timestamps matching backend timestamps?

I propose that what we need to do is get rid of the dangerous and
none-too-readable-anyway use of "int64" to declare replication-protocol
timestamps, and instead declare them as, say,

typedef struct RPTimestamp
{
int64 rptimestamp;
} RPTimestamp;

This will entail slightly more notation in the subroutines that actually
do something with the timestamp values, but it will make certain that
you can't assign or compare an RPTimestamp to a backend timestamp without
the compiler complaining about it.

regards, tom lane

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-02-17 18:21:25 Re: pg_recvlogical.c doesn't build with --disable-integer-datetimes
Previous Message Alvaro Herrera 2017-02-17 18:13:48 Re: pg_recvlogical.c doesn't build with --disable-integer-datetimes