Обсуждение: walprotocol.h vs frontends

I'm trying to make my streaming log receiver work properly with 9.1,
and have come across a couple of things. The first one that's causing
trouble is that the definition of the protocol is currently in
walprotocol.h, which is not include:able in a frontend application.
AFAICT, this is because it includes utils/timestamp.h, which doesn't
work. AFAICT, this means that anybody other than our own backend who
wants to talk our replication protocol has to copy the specific struct
defines they want in their own code. This seems like a really bad
idea. (In my case, it's the StandbyReplyMessage that I need, so I can
make my client not get killed by the default settings for timeout)

The basic reason for this is that we're putting TimestampTz fields in
the protocol. This also means that the protocol actually changes
definition depending on if the server is compiled with integer or
float timestamps. While the replication itself breaks if these are
different, this seems like a bad thing to expose in the protocol. It
also makes life a lot harder on third party tools.

Any ideas on what to do with this, other than me copying the struct
defs and changing them to int64 (which does work in my lab, but seems
like a horrible kludge).

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 15 August 2011 12:22, Magnus Hagander <magnus@hagander.net> wrote:
> The basic reason for this is that we're putting TimestampTz fields in
> the protocol. This also means that the protocol actually changes
> definition depending on if the server is compiled with integer or
> float timestamps.

Without commenting on what should be done in your specific case, I
wonder whether it's time to fully retire the deprecated double
representation of timestamps. Is anyone actually expected to rely on
their availability when 9.2 is released? This also caused difficulties
for Johann Oskarsson recently, during work on PL/Java.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Magnus Hagander <magnus@hagander.net> writes:
> I'm trying to make my streaming log receiver work properly with 9.1,
> and have come across a couple of things. The first one that's causing
> trouble is that the definition of the protocol is currently in
> walprotocol.h, which is not include:able in a frontend application.
> AFAICT, this is because it includes utils/timestamp.h, which doesn't
> work. AFAICT, this means that anybody other than our own backend who
> wants to talk our replication protocol has to copy the specific struct
> defines they want in their own code. This seems like a really bad
> idea. (In my case, it's the StandbyReplyMessage that I need, so I can
> make my client not get killed by the default settings for timeout)

> The basic reason for this is that we're putting TimestampTz fields in
> the protocol. This also means that the protocol actually changes
> definition depending on if the server is compiled with integer or
> float timestamps. While the replication itself breaks if these are
> different, this seems like a bad thing to expose in the protocol. It
> also makes life a lot harder on third party tools.

I don't really see why it matters, given that the data to be shipped is
also dependent on the timestamp format?

However, for a narrow fix, I could see moving the data type definition
to someplace with fewer dependencies.  Perhaps split it into a separate
file timestamp_type.h, or something like that.
        regards, tom lane

On Mon, Aug 15, 2011 at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> I'm trying to make my streaming log receiver work properly with 9.1,
>> and have come across a couple of things. The first one that's causing
>> trouble is that the definition of the protocol is currently in
>> walprotocol.h, which is not include:able in a frontend application.
>> AFAICT, this is because it includes utils/timestamp.h, which doesn't
>> work. AFAICT, this means that anybody other than our own backend who
>> wants to talk our replication protocol has to copy the specific struct
>> defines they want in their own code. This seems like a really bad
>> idea. (In my case, it's the StandbyReplyMessage that I need, so I can
>> make my client not get killed by the default settings for timeout)
>
>> The basic reason for this is that we're putting TimestampTz fields in
>> the protocol. This also means that the protocol actually changes
>> definition depending on if the server is compiled with integer or
>> float timestamps. While the replication itself breaks if these are
>> different, this seems like a bad thing to expose in the protocol. It
>> also makes life a lot harder on third party tools.
>
> I don't really see why it matters, given that the data to be shipped is
> also dependent on the timestamp format?

As an example, the stream receiver program needs to be able to
construct the status message and send back - thus needs to be able to
construct a TimestampTz. It never needs to look into the actual WAL
data - it just writes it to a file considering it to be a stream of
pure binary data.


> However, for a narrow fix, I could see moving the data type definition
> to someplace with fewer dependencies.  Perhaps split it into a separate
> file timestamp_type.h, or something like that.

Yes, that seems to fix the problem of timestamptz. See the attached
patch - seems ok?


I also ran into a similar problem with some WAL macro definitions that
are in xlog_internal.h. I've moved them to xlogdefs.h in the attached
xlog.diff file. Does that seem ok as well, or should I move them
somewhere else? (I don't need all those macros, but I moved the
complete block of macros wherever I needed one of them, because
otherwise it would be completely impossible to track). This is just a
simple move of the definitions, zero new logic added and zero removed.


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Aug 15, 2011 at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, for a narrow fix, I could see moving the data type definition
>> to someplace with fewer dependencies. �Perhaps split it into a separate
>> file timestamp_type.h, or something like that.

> Yes, that seems to fix the problem of timestamptz. See the attached
> patch - seems ok?

Don't think you should expose fsec_t, nor most of those macros.  The
foo_per_bar values are just namespace clutter.

> I also ran into a similar problem with some WAL macro definitions that
> are in xlog_internal.h. I've moved them to xlogdefs.h in the attached
> xlog.diff file. Does that seem ok as well, or should I move them
> somewhere else?

I don't like the idea of exposing those to frontends, either.  What do
you actually *need* out of that, and why?
        regards, tom lane

On Mon, Aug 15, 2011 at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Mon, Aug 15, 2011 at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> However, for a narrow fix, I could see moving the data type definition
>>> to someplace with fewer dependencies.  Perhaps split it into a separate
>>> file timestamp_type.h, or something like that.
>
>> Yes, that seems to fix the problem of timestamptz. See the attached
>> patch - seems ok?
>
> Don't think you should expose fsec_t, nor most of those macros.  The
> foo_per_bar values are just namespace clutter.

Hmm, ok. I just went for what seemed like a reasonable subset. I do
also need the SECS_PER_DAY and those constants in order to be able to
convert the timestamp value. That led me to include the other
foo_per_bar so they are all in one place - seems wrong to have them
split up.



>> I also ran into a similar problem with some WAL macro definitions that
>> are in xlog_internal.h. I've moved them to xlogdefs.h in the attached
>> xlog.diff file. Does that seem ok as well, or should I move them
>> somewhere else?
>
> I don't like the idea of exposing those to frontends, either.  What do
> you actually *need* out of that, and why?

PrevLogSeg - which also brings in XLogSegsPerFile.
XLogFileName
XLogFileSize - which brings in XLogSegsPerFile and XLogSegSize

PrevLogSeg should be self explaining, as should xlogfilename.

XLogFileSize is required by XLByteAdvance() which is already in
xlogdefs.h - so xlogdefs.h actually has a hidden dependency on
xlog_internal.h here today.

I can certainly separate those out, but it seemed more clean to move
the whole block they were in.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 11-08-15 10:00 AM, Peter Geoghegan wrote:
>
> Without commenting on what should be done in your specific case, I
> wonder whether it's time to fully retire the deprecated double
> representation of timestamps. Is anyone actually expected to rely on
> their availability when 9.2 is released? This also caused difficulties
> for Johann Oskarsson recently, during work on PL/Java.

This would mean that anyone using the floating point timestamps today 
won't be able to use pg_upgrade to upgrade to whichever version we 
remove them from.  8.3 had float based timestamps as the default and I 
suspect many installations with the default 8.3 settings have been 
upgraded via pg_upgrade to 9.0 built the old timestamps representation.

Steve

On 15.08.2011 18:54, Magnus Hagander wrote:
> On Mon, Aug 15, 2011 at 16:53, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>> Magnus Hagander<magnus@hagander.net>  writes:
>>> I also ran into a similar problem with some WAL macro definitions that
>>> are in xlog_internal.h. I've moved them to xlogdefs.h in the attached
>>> xlog.diff file. Does that seem ok as well, or should I move them
>>> somewhere else?
>>
>> I don't like the idea of exposing those to frontends, either.  What do
>> you actually *need* out of that, and why?
>
> PrevLogSeg - which also brings in XLogSegsPerFile.
> XLogFileName
> XLogFileSize - which brings in XLogSegsPerFile and XLogSegSize
>
> PrevLogSeg should be self explaining, as should xlogfilename.
>
> XLogFileSize is required by XLByteAdvance() which is already in
> xlogdefs.h - so xlogdefs.h actually has a hidden dependency on
> xlog_internal.h here today.
>
> I can certainly separate those out, but it seemed more clean to move
> the whole block they were in.

Perhaps we should change the protocol so that it explicitly says which 
file the streamed piece of WAL belongs to. That way a client could write 
it to the correct file without knowing about all those macros.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Perhaps we should change the protocol so that it explicitly says which 
> file the streamed piece of WAL belongs to. That way a client could write 
> it to the correct file without knowing about all those macros.

Yeah, maybe.  Otherwise, all that logic is not only implicitly part of
the protocol, but essentially impossible to change because it'll be
hard-coded into clients.  I wasn't too worried about this before because
I supposed that only walsender and walreceiver really needed to know
anything about it.  If Magnus is busy coding something else that speaks
that protocol, we have a problem.  (Assuming we believe his use-case is
sane, that is.)
        regards, tom lane

On Mon, Aug 15, 2011 at 18:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> Perhaps we should change the protocol so that it explicitly says which
>> file the streamed piece of WAL belongs to. That way a client could write
>> it to the correct file without knowing about all those macros.
>
> Yeah, maybe.  Otherwise, all that logic is not only implicitly part of
> the protocol, but essentially impossible to change because it'll be
> hard-coded into clients.  I wasn't too worried about this before because
> I supposed that only walsender and walreceiver really needed to know

Wouldn't doing that cause significant overhead? Every single packet
would have to contain the filename, since the wal stream isn't
depending onthe filenames in where it cuts off. Also, the way it is
now a single packet on the replication stream can span multiple WAL
files... (This is the bug in my previous version that I've been able
to fix now)

> anything about it.  If Magnus is busy coding something else that speaks
> that protocol, we have a problem.  (Assuming we believe his use-case is
> sane, that is.)

I am. And said program has existed for almost a year, IIRC. It has
even been on a commitfest for 9.1, but had a bug and I didn't have
time to fix it (it still worked in a lot of cases, but had
cornercases). And a lot of different people have requested said
functionality, so at least they believe it's a usecase (the usecase
being able to stream your backups at a smaller interval than 16Mb).


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 15 August 2011 16:56, Steve Singer <ssinger@ca.afilias.info> wrote:
> This would mean that anyone using the floating point timestamps today won't
> be able to use pg_upgrade to upgrade to whichever version we remove them
> from.  8.3 had float based timestamps as the default and I suspect many
> installations with the default 8.3 settings have been upgraded via
> pg_upgrade to 9.0 built the old timestamps representation.

Really? I find that slightly surprising, considering that a quick look
at master's timestamp.c suggests that the choice to use the in64
representation over the double representation is entirely a case of
compile time either/or. There is no apparent fall-back to the double
representation available to binaries built without
--disable-integer-datetimes.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Aug 15, 2011 at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Don't think you should expose fsec_t, nor most of those macros. �The
>> foo_per_bar values are just namespace clutter.

> Hmm, ok. I just went for what seemed like a reasonable subset. I do
> also need the SECS_PER_DAY and those constants in order to be able to
> convert the timestamp value.

Uh ... "convert"?  That seems to require a whole lot more knowledge
about timestamps than what is exposed by the proposed new header.
I thought you were just hoping to compile a struct containing a
TimestampTz field, not actually do anything with that field.

>> I don't like the idea of exposing those to frontends, either. �What do
>> you actually *need* out of that, and why?

> PrevLogSeg - which also brings in XLogSegsPerFile.
> XLogFileName
> XLogFileSize - which brings in XLogSegsPerFile and XLogSegSize

> PrevLogSeg should be self explaining, as should xlogfilename.

> XLogFileSize is required by XLByteAdvance() which is already in
> xlogdefs.h - so xlogdefs.h actually has a hidden dependency on
> xlog_internal.h here today.

Yeah, and the question remains why is "frontend" code using any of that?

I'm inclined to think that there is zero chance of this code being
"frontend" in the sense of being at any real arm's length from backend
implementation details, and so you should just forget these header
maneuvers and do what pg_resetxlog.c does, ie

/** We have to use postgres.h not postgres_fe.h here, because there's so much* backend-only stuff in the XLOG include
fileswe need.  But we need a* frontend-ish environment otherwise.    Hence this ugly hack.*/
 
#define FRONTEND 1

#include "postgres.h"

        regards, tom lane

On Mon, Aug 15, 2011 at 18:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Mon, Aug 15, 2011 at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Don't think you should expose fsec_t, nor most of those macros.  The
>>> foo_per_bar values are just namespace clutter.
>
>> Hmm, ok. I just went for what seemed like a reasonable subset. I do
>> also need the SECS_PER_DAY and those constants in order to be able to
>> convert the timestamp value.
>
> Uh ... "convert"?  That seems to require a whole lot more knowledge
> about timestamps than what is exposed by the proposed new header.
> I thought you were just hoping to compile a struct containing a
> TimestampTz field, not actually do anything with that field.

Um, bad choice of words. I don't need to convert, really, but I do
need to stick the current timestamp into a TimestampTz field.

>>> I don't like the idea of exposing those to frontends, either.  What do
>>> you actually *need* out of that, and why?
>
>> PrevLogSeg - which also brings in XLogSegsPerFile.
>> XLogFileName
>> XLogFileSize - which brings in XLogSegsPerFile and XLogSegSize
>
>> PrevLogSeg should be self explaining, as should xlogfilename.
>
>> XLogFileSize is required by XLByteAdvance() which is already in
>> xlogdefs.h - so xlogdefs.h actually has a hidden dependency on
>> xlog_internal.h here today.
>
> Yeah, and the question remains why is "frontend" code using any of that?
>
> I'm inclined to think that there is zero chance of this code being
> "frontend" in the sense of being at any real arm's length from backend
> implementation details, and so you should just forget these header
> maneuvers and do what pg_resetxlog.c does, ie
>
> /*
>  * We have to use postgres.h not postgres_fe.h here, because there's so much
>  * backend-only stuff in the XLOG include files we need.  But we need a
>  * frontend-ish environment otherwise.  Hence this ugly hack.
>  */
> #define FRONTEND 1
>
> #include "postgres.h"

That does seem to work, yes. I completely forgot about that hack. And
I agree that's probably th eway to do it...

Now I just need to figure out how to get to GetCurrentTimestamp() to
be accessible - or just copy it over under a different name...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 11-08-15 12:33 PM, Peter Geoghegan wrote:
> On 15 August 2011 16:56, Steve Singer<ssinger@ca.afilias.info>  wrote:
>> This would mean that anyone using the floating point timestamps today won't
>> be able to use pg_upgrade to upgrade to whichever version we remove them
>> from.  8.3 had float based timestamps as the default and I suspect many
>> installations with the default 8.3 settings have been upgraded via
>> pg_upgrade to 9.0 built the old timestamps representation.
>
> Really? I find that slightly surprising, considering that a quick look
> at master's timestamp.c suggests that the choice to use the in64
> representation over the double representation is entirely a case of
> compile time either/or. There is no apparent fall-back to the double
> representation available to binaries built without
> --disable-integer-datetimes.
>

I *think* the default on 8.3 was float based timestamps.  If you want to 
upgrade a system running 8.3 (that uses float based timestamps) in 
using pg_upgrade you must compile 9.0 (or 8.4 or 9.1) with 
--disable-integer-datetimes.  If at some point in the future you then 
want to upgrade to 9.2 with pg_upgrade you will again need to build 9.2 
with --disable-integer-datetimes.    If we remove the code for floating 
point representations of datetime then you won't be able to do that.


Steve

Steve Singer <ssinger@ca.afilias.info> writes:
> On 11-08-15 12:33 PM, Peter Geoghegan wrote:
>> On 15 August 2011 16:56, Steve Singer<ssinger@ca.afilias.info>  wrote:
>>> This would mean that anyone using the floating point timestamps today won't
>>> be able to use pg_upgrade to upgrade to whichever version we remove them
>>> from.  8.3 had float based timestamps as the default and I suspect many
>>> installations with the default 8.3 settings have been upgraded via
>>> pg_upgrade to 9.0 built the old timestamps representation.

>> Really?

> I *think* the default on 8.3 was float based timestamps.

Yeah, it was.  Also, the fact that the source distribution switched
defaults at 8.4 has not got a lot to do with what individual binary
distributions did --- some switched earlier, some may still not have
done so.  And of course individual users building from source were
and still are free to make their own choice.

It's going to be quite a long time before we can contemplate removing
float timestamp support altogether.
        regards, tom lane

On 15 August 2011 18:09, Steve Singer <ssinger@ca.afilias.info> wrote:
>> Really? I find that slightly surprising, considering that a quick look
>> at master's timestamp.c suggests that the choice to use the in64
>> representation over the double representation is entirely a case of
>> compile time either/or. There is no apparent fall-back to the double
>> representation available to binaries built without
>> --disable-integer-datetimes.
>>
>
> I *think* the default on 8.3 was float based timestamps.

Yes, it is.

> If you want to upgrade a system running 8.3 (that uses float based timestamps) in using
> pg_upgrade you must compile 9.0 (or 8.4 or 9.1) with
> --disable-integer-datetimes.  If at some point in the future you then want
> to upgrade to 9.2 with pg_upgrade you will again need to build 9.2 with
> --disable-integer-datetimes.    If we remove the code for floating point
> representations of datetime then you won't be able to do that.

I'm pretty surprised that pg_upgrade pushes that onus onto its users -
for many users, the need to build their own binaries is a greater
barrier to upgrading than doing a logical restore. Maybe that's simply
considered a matter for package managers to worry about, but that
doesn't sit well with me.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Aug 15, 2011 at 5:15 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Aug 15, 2011 at 18:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>>> Perhaps we should change the protocol so that it explicitly says which
>>> file the streamed piece of WAL belongs to. That way a client could write
>>> it to the correct file without knowing about all those macros.
>>
>> Yeah, maybe.  Otherwise, all that logic is not only implicitly part of
>> the protocol, but essentially impossible to change because it'll be
>> hard-coded into clients.  I wasn't too worried about this before because
>> I supposed that only walsender and walreceiver really needed to know
>
> Wouldn't doing that cause significant overhead? Every single packet
> would have to contain the filename, since the wal stream isn't
> depending onthe filenames in where it cuts off. Also, the way it is
> now a single packet on the replication stream can span multiple WAL
> files... (This is the bug in my previous version that I've been able
> to fix now)

Why not have a specific protocol message to indicate a change of filename?

I don't mean a WAL message, I mean a streaming protocol message.

At present the WALSender only sends from one file at a time, so
sending a message when we open a new file would be straightforward.

Magnus needs some things to make this work for 9.0/9.1, but we don't
need to change 9.2 to allow that, we just need to copy the definitions
for those now-fixed releases.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Aug 15, 2011 at 21:12, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Mon, Aug 15, 2011 at 5:15 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Mon, Aug 15, 2011 at 18:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>>>> Perhaps we should change the protocol so that it explicitly says which
>>>> file the streamed piece of WAL belongs to. That way a client could write
>>>> it to the correct file without knowing about all those macros.
>>>
>>> Yeah, maybe.  Otherwise, all that logic is not only implicitly part of
>>> the protocol, but essentially impossible to change because it'll be
>>> hard-coded into clients.  I wasn't too worried about this before because
>>> I supposed that only walsender and walreceiver really needed to know
>>
>> Wouldn't doing that cause significant overhead? Every single packet
>> would have to contain the filename, since the wal stream isn't
>> depending onthe filenames in where it cuts off. Also, the way it is
>> now a single packet on the replication stream can span multiple WAL
>> files... (This is the bug in my previous version that I've been able
>> to fix now)
>
> Why not have a specific protocol message to indicate a change of filename?
>
> I don't mean a WAL message, I mean a streaming protocol message.

That we could have, and it would work as long as it's always sent as
the first packet in any stream.

If we do that, we should probably make it a general metadata package -
including things like segment size as well...


> At present the WALSender only sends from one file at a time, so
> sending a message when we open a new file would be straightforward.

Are you sure? We can receive a single message spanning multiple files...


> Magnus needs some things to make this work for 9.0/9.1, but we don't
> need to change 9.2 to allow that, we just need to copy the definitions
> for those now-fixed releases.

The hack from pg_resetxlog with a manual #define FRONTEND 1 will work
for current releases, I think. And it will work for future ones as
well - but you'll be in a position where using the log streaming tool
built with different parameters (like xlog seg size) than the server
will not work - I'm not sure that's a problem big enough to worry
about, though...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
>> Why not have a specific protocol message to indicate a change of filename?
>> I don't mean a WAL message, I mean a streaming protocol message.
>
> That we could have, and it would work as long as it's always sent as
> the first packet in any stream.
>
> If we do that, we should probably make it a general metadata package -
> including things like segment size as well...

I think this could be made an opt-in: the streaming client would have to
ask for it when connecting to the walsender (or maybe whenever in the
connection stream, I don't know).

Then it can even be backported, and it won't add anything to core
clients streaming data.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On Mon, Aug 15, 2011 at 10:32 PM, Magnus Hagander <magnus@hagander.net> wrote:

>> At present the WALSender only sends from one file at a time, so
>> sending a message when we open a new file would be straightforward.
>
> Are you sure? We can receive a single message spanning multiple files...

You're right. That was the way the original code ran but I thought we
had stopped that when we introduced MAX_SEND_SIZE. The comment says
"don't cross a logfile boundary within one message". What that
actually does is prevent us incrementing a logid value, which happens
every 255 files. I read that as meaning "WAL file" which is not what
it means at all.

So right now what we do is allow a single packet to span multiple
files, but since MAX_SEND_SIZE is 128KB it will always be smaller than
a single file, so we can only ever span two files at most.

That is all just a little bizarre, especially since libpq sends data
in 8KB chunks anyway.

So I suggest we change XLogSend() so that it only ever sends up to the
end of a file. That way all "w" messages will relate to just one file,
and we can have another message to initiate a new file. And then, as
you say, give full metadata for the new file.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Aug 16, 2011 at 00:05, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Mon, Aug 15, 2011 at 10:32 PM, Magnus Hagander <magnus@hagander.net> wrote:
>
>>> At present the WALSender only sends from one file at a time, so
>>> sending a message when we open a new file would be straightforward.
>>
>> Are you sure? We can receive a single message spanning multiple files...
>
> You're right. That was the way the original code ran but I thought we
> had stopped that when we introduced MAX_SEND_SIZE. The comment says
> "don't cross a logfile boundary within one message". What that
> actually does is prevent us incrementing a logid value, which happens
> every 255 files. I read that as meaning "WAL file" which is not what
> it means at all.

That has bitten me many times. log file != wal file.. It's not exactly
intuitive.


> So right now what we do is allow a single packet to span multiple
> files, but since MAX_SEND_SIZE is 128KB it will always be smaller than
> a single file, so we can only ever span two files at most.

Unless someone has tweaked their xlog segment size to insane values.

I ended up writing a loop that can deal with it spanning >2 files as
well - but that also turned out to be less code than the special case
for 2 files, so I'll be keeping that :-)


> That is all just a little bizarre, especially since libpq sends data
> in 8KB chunks anyway.
>
> So I suggest we change XLogSend() so that it only ever sends up to the
> end of a file. That way all "w" messages will relate to just one file,
> and we can have another message to initiate a new file. And then, as
> you say, give full metadata for the new file.

That could be an idea in itself. It is not necessary for the log
receiver, since it can use the #define FRONTEND hack (which only works
if you build inside the source tree of course, not if it's an
extension) but it might make the protocol simpler to deal with for
third parties who want to do something similar.


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On mån, 2011-08-15 at 18:39 +0100, Peter Geoghegan wrote:
> > If you want to upgrade a system running 8.3 (that uses float based
> timestamps) in using
> > pg_upgrade you must compile 9.0 (or 8.4 or 9.1) with
> > --disable-integer-datetimes.  If at some point in the future you
> then want
> > to upgrade to 9.2 with pg_upgrade you will again need to build 9.2
> with
> > --disable-integer-datetimes.    If we remove the code for floating
> point
> > representations of datetime then you won't be able to do that.
> 
> I'm pretty surprised that pg_upgrade pushes that onus onto its users -
> for many users, the need to build their own binaries is a greater
> barrier to upgrading than doing a logical restore. Maybe that's simply
> considered a matter for package managers to worry about, but that
> doesn't sit well with me.

Well, pg_upgrade only moves the heap files, it doesn't look into them or
change them.

Possibly, this sort of issue could be better handled in the future by
making this a cluster, database, or table flag instead of a compile-time
option.  That way, at least newly created things could move to the new
recommended behavior.  The way it is set up now, we will possibly never
get rid of the legacy behavior, unless we break pg_upgrade at some
point.

On Tue, Aug 16, 2011 at 9:35 AM, Magnus Hagander <magnus@hagander.net> wrote:

>> So right now what we do is allow a single packet to span multiple
>> files, but since MAX_SEND_SIZE is 128KB it will always be smaller than
>> a single file, so we can only ever span two files at most.
>
> Unless someone has tweaked their xlog segment size to insane values.

We should limit MAX_SEND_SIZE to be same or less than WAL_SEGMENT_SIZE.

We gain nothing by continuing to allow the existing code to work in
the way it does.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services