Re: [HACKERS] Unportable coding in reorderbuffer.h
On 2014-03-07 13:27:28 -0500, Tom Lane wrote: > Andres Freund writes: > > A patch fixing this is attached. I've added some more local variables to > > deal with the longer lines... > > Since I've got a compiler in captivity that complains about this, > I'll take care of checking and committing this patch. Thanks. The tests specific for the feature can be run with (cd contrib/test_decoding && make -s check), as they require non-default PGC_POSTMASTER settings. > I still think it might be a good idea to spin up a buildfarm member > running "gcc -ansi -pedantic", even if we don't see that as a particularly > useful case in practice. Thoughts? I tried to compile with both -ansi -pedantic -std=c90 to catch potential further issues, but at least my gcc version makes the output completely drown in various warnings. Some can be individually disabled, but lots of them seem to be only be covered by pretty general warning categories. -ansi -std=c90 -Wno-variadic-macros works reasonably well tho. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
Andres Freund writes: > A patch fixing this is attached. I've added some more local variables to > deal with the longer lines... Since I've got a compiler in captivity that complains about this, I'll take care of checking and committing this patch. I still think it might be a good idea to spin up a buildfarm member running "gcc -ansi -pedantic", even if we don't see that as a particularly useful case in practice. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
Hi,
On 2014-03-06 02:39:37 +0100, Andres Freund wrote:
> Unless somebody protests I'll try to get the remaining walsender and
> docs patches ready before sending in the patch fixing this as it's not
> disturbing the buildfarm. I'll be onsite/travelling tomorrow; so I am
> not sure I'll be done then, but friday it is.
A patch fixing this is attached. I've added some more local variables to
deal with the longer lines...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 875eda2d163e38e6533bf6cf8e6e6417aad0421b Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Thu, 6 Mar 2014 02:00:47 +0100
Subject: [PATCH] Remove unportable use of anonymous unions from
reorderbuffer.h.
In b89e151054a I had assumed it was ok to use anonymous unions as
struct members, but while a longstanding extension in many compilers,
it's only been standardized in C11.
To fix, remove one of the anonymous unions which tried to hide some
implementation specific enum values and give the other a name. The
latter unfortunately requires changes in output plugins, but since the
feature has only been added a few days ago...
Per complaint from Tom Lane.
---
contrib/test_decoding/test_decoding.c | 18 +-
src/backend/replication/logical/decode.c| 28 +--
src/backend/replication/logical/reorderbuffer.c | 283
src/include/replication/reorderbuffer.h | 39 ++--
4 files changed, 186 insertions(+), 182 deletions(-)
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index ea463fb..e356c7c 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -358,43 +358,45 @@ pg_decode_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
{
case REORDER_BUFFER_CHANGE_INSERT:
appendStringInfoString(ctx->out, " INSERT:");
- if (change->tp.newtuple == NULL)
+ if (change->data.tp.newtuple == NULL)
appendStringInfoString(ctx->out, " (no-tuple-data)");
else
tuple_to_stringinfo(ctx->out, tupdesc,
- &change->tp.newtuple->tuple,
+ &change->data.tp.newtuple->tuple,
false);
break;
case REORDER_BUFFER_CHANGE_UPDATE:
appendStringInfoString(ctx->out, " UPDATE:");
- if (change->tp.oldtuple != NULL)
+ if (change->data.tp.oldtuple != NULL)
{
appendStringInfoString(ctx->out, " old-key:");
tuple_to_stringinfo(ctx->out, tupdesc,
- &change->tp.oldtuple->tuple,
+ &change->data.tp.oldtuple->tuple,
true);
appendStringInfoString(ctx->out, " new-tuple:");
}
- if (change->tp.newtuple == NULL)
+ if (change->data.tp.newtuple == NULL)
appendStringInfoString(ctx->out, " (no-tuple-data)");
else
tuple_to_stringinfo(ctx->out, tupdesc,
- &change->tp.newtuple->tuple,
+ &change->data.tp.newtuple->tuple,
false);
break;
case REORDER_BUFFER_CHANGE_DELETE:
appendStringInfoString(ctx->out, " DELETE:");
/* if there was no PK, we only know that a delete happened */
- if (change->tp.oldtuple == NULL)
+ if (change->data.tp.oldtuple == NULL)
appendStringInfoString(ctx->out, " (no-tuple-data)");
/* In DELETE, only the replica identity is present; display that */
else
tuple_to_stringinfo(ctx->out, tupdesc,
- &change->tp.oldtuple->tuple,
+ &change->data.tp.oldtuple->tuple,
true);
break;
+ default:
+ Assert(false);
}
MemoryContextSwitchTo(old);
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index af3948e..414cfa9 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -586,17 +586,17 @@ DecodeInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
change = ReorderBufferGetChange(ctx->reorder);
change->action = REORDER_BUFFER_CHANGE_INSERT;
- memcpy(&change->tp.relnode, &xlrec->target.node, sizeof(RelFileNode));
+ memcpy(&change->data.tp.relnode, &xlrec->target.node, sizeof(RelFileNode));
if (xlrec->flags & XLOG_HEAP_CONTAINS_NEW_TUPLE)
{
Assert(r->xl_len > (SizeOfHeapInsert + SizeOfHeapHeader));
- change->tp.newtuple = ReorderBufferGetTupleBuf(ctx->reorder);
+ change->data.tp.newtuple = ReorderBufferGetTupleBuf(ctx->reorder);
DecodeXLogTuple((char *) xlrec + SizeOfHeapInsert,
r->xl_len - SizeOfHeapInsert,
- change->tp.newtuple);
+ change->data.tp.newtuple);
}
ReorderBufferQueueChange(ctx->reorder, r->xl_xid, buf->origptr, change);
@@ -626,7 +626,7 @@ DecodeUpdate(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
change = ReorderBufferGetChange(ctx->reorder);
change->action = REORDER_BUFFER_CHANGE_UPDATE;
- memcpy(&change->tp.relnode, &xlrec->target.node, sizeof(RelFileNode));
+ memcpy(&change->data.tp.relnode, &xlrec->target.node, sizeof(RelFileNode));
data = (char *) &xl
Re: [HACKERS] Unportable coding in reorderbuffer.h
On 2014-03-05 20:02:56 -0500, Robert Haas wrote: > On Wed, Mar 5, 2014 at 6:50 PM, Andres Freund wrote: > Urgh. I know that isn't per project style, but I just plain missed it > while staring at these patches. At one point I thought of complaining > that separating the public and private values was not a worthwhile > endeavor, but I don't think I ever did. Still, I agree with Tom's > suggestion of dumping the distinction. Ok, convinced, consider it dumped. Unless somebody protests I'll try to get the remaining walsender and docs patches ready before sending in the patch fixing this as it's not disturbing the buildfarm. I'll be onsite/travelling tomorrow; so I am not sure I'll be done then, but friday it is. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
On 2014-03-05 20:03:16 -0500, Tom Lane wrote: > However, this is probably a bit beside the point. I'm quite prepared > to believe that nobody uses gcc < 4.0 anymore. The question is what > older non-gcc compilers are still out there, and can we either get hold > of them for the buildfarm, or trust that a really old gcc will complain > about the same things they would? I suspect that most of the candidates > would be proprietary compilers, so short of shelling out license fees > I think we might be stuck with using old gcc as a proxy. I wonder if it makes sense to send out an -announce email asking for critters if they want $platform to stay supported. No response doesn't imply explicitly desupporting $platform... The channels where I recall request for critters on old platforms being made don't have a very high circulation (e.g. -bugs). > > I personally think it's time to dump some older compiler versions, and > > adopt at least individual C99 features (e.g. static inlines). > > Meh. In the first place, what you want is not C99 inlines it's GNU > inlines; the standard's version is brain-dead. That's true for several inline variants (extern inline, inline without extern), but unless I am severely misremembering not for static inlines. And static inline is what I am interested in. > But I'm not prepared to declare us a GCC-only shop. All platforms, even the hpux critter that existed till some time ago, do support static inline. It's only that the hpux critter warned about unused functions, but even that could have been disabled with a compiler flag. > In the second place, we already have a > workable if slightly klugy solution for GNU inlines without assuming > all compilers do that. Meh squared. As the person first starting with the STATIC_IF_INLINE thing I am readily declaring it a utterly horrible crock. And not one suited for all things. I very much want to replace some of the uglier macros with inline functions. Some are very, very hard to understand and give utterly horrible error messages that are very hard to understand, even for experienced people. And several of those cannot be replaced by a extern function without indirectly making the respective non-static-inline-supporting platforms indirectly unsupported. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
Andres Freund writes: > On 2014-03-05 19:12:15 -0500, Tom Lane wrote: >> I'm surprised too; I had thought we still had some critters running >> hoary compilers. We need to do something about that if we actually >> believe in C90-compiler support. > What version was the gcc that triggered the error? That was the 2.95.3 I have on my HPPA box. I don't have any 3.x versions in captivity; the next oldest I have is 4.0.1 on a Mac (running Leopard or thereabouts), and it seems to take this code. > I have to admit that I am really not interested in supporting gcc 2.95 , > that thing is just too old (nearly 15 years!) and buggy. [ shrug... ] In 15 years, the only problem I've seen with 2.95.3 is that it's prone to complaining about variables-clobbered-by-longjmp that no other compiler is unhappy with. Maybe the HPPA build is less buggy due to less aggressive optimization? I usually use -O1 with it, and that backend might be less tense than the Intel backend anyway. However, this is probably a bit beside the point. I'm quite prepared to believe that nobody uses gcc < 4.0 anymore. The question is what older non-gcc compilers are still out there, and can we either get hold of them for the buildfarm, or trust that a really old gcc will complain about the same things they would? I suspect that most of the candidates would be proprietary compilers, so short of shelling out license fees I think we might be stuck with using old gcc as a proxy. As I said, I've more often than not found that things 2.95.3 will take don't cause problems elsewhere. > I personally think it's time to dump some older compiler versions, and > adopt at least individual C99 features (e.g. static inlines). Meh. In the first place, what you want is not C99 inlines it's GNU inlines; the standard's version is brain-dead. But I'm not prepared to declare us a GCC-only shop. In the second place, we already have a workable if slightly klugy solution for GNU inlines without assuming all compilers do that. I don't see a need to throw that overboard. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
On Wed, Mar 5, 2014 at 6:50 PM, Andres Freund wrote:
> On 2014-03-05 17:40:56 -0500, Tom Lane wrote:
>> I don't believe that this is legal per C90:
>>
>> typedef struct ReorderBufferChange
>> {
>> XLogRecPtrlsn;
>>
>> /* type of change */
>> union
>> {
>> enum ReorderBufferChangeType action;
>> /* do not leak internal enum values to the outside */
>> intaction_internal;
>> };
>>
>> ...
>>
>> That union field needs a name.
>
> You're absolutely right.
>
>> Our project standard is we compile on C90 compilers, and we're not
>> moving that goalpost just to save you a couple of keystrokes.
>
> While it's a good idea to try to save keystrokes the actual reason is
> that I just had somehow forgotten that it hasn't always been
> supported. I think it's not even C99, but C11...
Urgh. I know that isn't per project style, but I just plain missed it
while staring at these patches. At one point I thought of complaining
that separating the public and private values was not a worthwhile
endeavor, but I don't think I ever did. Still, I agree with Tom's
suggestion of dumping the distinction.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
On 2014-03-05 19:12:15 -0500, Tom Lane wrote: > Andres Freund writes: > > On 2014-03-05 17:40:56 -0500, Tom Lane wrote: > >> By the time you get done fixing the portability issue, I suspect you > >> won't have a union at all for the first case. > > > You might be right. I'd rather not leak the internal enum values to the > > public though, so there are two choices: > > * define as enum, but store values in the that aren't actually valid > > members. IIRC that's legal, even if not pretty. Anything outside > > reorderbuffer.c doesn't ever see the values that aren't enum values. > > * define it as an int, but suggest casting to the enum inside switch() > > in examples/docs. > > Meh. I think you're being *way* too cute here. I'd just put all the > values in one enum declaration, and use comments and/or naming convention > to mark some of them as internal and not meant to be used outside the > reorderbuffer stuff. That will for one thing allow gdb to print all > the values properly. And somebody who's bound and determined to violate > the abstraction could do it anyway, no? The reason I'd like to avoid the internal members in the public enum isn't so much that I want to avoid the internal names being visible to the outside, but that I find it very helpful to see compile time warnings about public enum values not being handled in output plugins. I am pretty sure we're going to add further features in the not too far away futures and it seems nicer to be able to warn that way. But I guess it's too much work, for not enough benefit :( > >> What drew my attention to it was an older gcc version complaining > >> thusly: [errors] > > > And there I was, suprised it hadn't turned the buildfarm red. > > I'm surprised too; I had thought we still had some critters running > hoary compilers. We need to do something about that if we actually > believe in C90-compiler support. What version was the gcc that triggered the error? I have to admit that I am really not interested in supporting gcc 2.95 , that thing is just too old (nearly 15 years!) and buggy. But it's not a small step from desupporting 2.95 to having gcc 3.4 (protosciurus, narwahl) as the minimum. And it should be a conscious not implicit decision. It'd be really helpful to have some more buildfarm animals running real-world relevant older compilers. I'd be surprised if this is the only such issue lurking around. As I've compiled it anyway while thinking about gcc versions, here's the lifetimes of various gcc releases: version first version last version --- 2.95July 31, 1999 March 16, 2001 3.0 June 18, 2001 December 20, 2001 3.1 May 15, 2002July 27, 2002 3.2 August 14, 2002 April 25, 2003 3.3 May 14, 2003May 3, 2005 3.4 April 18, 2004 March 6, 2006 4.0 April 20, 2005 January 31, 2007 4.1 February 28, 2006 February 13, 2007 4.2 May 13, 2007May 19, 2008 4.3 March 5, 2008 Jun 27, 2011 4.4 April 21, 2009 March 13, 2012 4.5 April 14, 2010 Jul 2, 2012 4.6 March 25, 2011 April 12, 2013 4.7 March 22, 2012 - 4.8 March 22, 2013 - --- I personally think it's time to dump some older compiler versions, and adopt at least individual C99 features (e.g. static inlines). Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
Andres Freund writes: > On 2014-03-05 17:40:56 -0500, Tom Lane wrote: >> By the time you get done fixing the portability issue, I suspect you >> won't have a union at all for the first case. > You might be right. I'd rather not leak the internal enum values to the > public though, so there are two choices: > * define as enum, but store values in the that aren't actually valid > members. IIRC that's legal, even if not pretty. Anything outside > reorderbuffer.c doesn't ever see the values that aren't enum values. > * define it as an int, but suggest casting to the enum inside switch() > in examples/docs. Meh. I think you're being *way* too cute here. I'd just put all the values in one enum declaration, and use comments and/or naming convention to mark some of them as internal and not meant to be used outside the reorderbuffer stuff. That will for one thing allow gdb to print all the values properly. And somebody who's bound and determined to violate the abstraction could do it anyway, no? >> What drew my attention to it was an older gcc version complaining >> thusly: [errors] > And there I was, suprised it hadn't turned the buildfarm red. I'm surprised too; I had thought we still had some critters running hoary compilers. We need to do something about that if we actually believe in C90-compiler support. I experimented a little bit and found that modern gcc with -ansi (aka -std=c89) will complain about this particular issue, and it also complains about // comments which are a perpetual hazard; but it is still happy about a lot of other not-C90 things like flexible array members. We could try spinning up a buildfarm critter with -ansi -pedantic but I'm not sure if that's much better. The GCC man page warns that these options are not meant to be a strict check for C90 standard compliance, and anyway the important question is not whether gcc with options that nobody uses will take our code, its whether old compilers that are still in real use will take it. For a long time I was using gcc 2.95.3 on my HPPA dinosaur as a reference for hoary-compiler behavior. That machine is now flaky enough that I don't want to keep it turned on 24/7, but maybe I could compile up 2.95.3 on some other box and start a buildfarm critter. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unportable coding in reorderbuffer.h
On 2014-03-05 17:40:56 -0500, Tom Lane wrote:
> I don't believe that this is legal per C90:
>
> typedef struct ReorderBufferChange
> {
> XLogRecPtrlsn;
>
> /* type of change */
> union
> {
> enum ReorderBufferChangeType action;
> /* do not leak internal enum values to the outside */
> intaction_internal;
> };
>
> ...
>
> That union field needs a name.
You're absolutely right.
> Our project standard is we compile on C90 compilers, and we're not
> moving that goalpost just to save you a couple of keystrokes.
While it's a good idea to try to save keystrokes the actual reason is
that I just had somehow forgotten that it hasn't always been
supported. I think it's not even C99, but C11...
> By the time you get done fixing the portability issue, I suspect you
> won't have a union at all for the first case.
You might be right. I'd rather not leak the internal enum values to the
public though, so there are two choices:
* define as enum, but store values in the that aren't actually valid
members. IIRC that's legal, even if not pretty. Anything outside
reorderbuffer.c doesn't ever see the values that aren't enum values.
* define it as an int, but suggest casting to the enum inside switch()
in examples/docs.
> I'm not real sure that
> you need a union for the second case either --- is it really important
> to shave a few bytes from the size of this struct? So you don't
> necessarily need to do a notation change for the second union.
I think it's allocated frequently enough to warrant that
unfortunately. Changing that will be fun.
> What drew my attention to it was an older gcc version complaining
> thusly: [errors]
And there I was, suprised it hadn't turned the buildfarm red.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Unportable coding in reorderbuffer.h
I don't believe that this is legal per C90:
typedef struct ReorderBufferChange
{
XLogRecPtrlsn;
/* type of change */
union
{
enum ReorderBufferChangeType action;
/* do not leak internal enum values to the outside */
intaction_internal;
};
...
That union field needs a name. Yeah, I realize this makes references
to the contained fields more verbose. Tough. Our project standard
is we compile on C90 compilers, and we're not moving that goalpost
just to save you a couple of keystrokes.
Worse, this isn't portable even if you assume a C99 compiler. There is
nothing at all constraining the compiler to make the enum field the same
size as the int field, and if they're not, reading the int will not yield
the same value you wrote as an enum (or vice versa). It might've
accidentally failed to fail on the compilers you tested on, but it's
still wrong.
The other nameless union in ReorderBufferChange needs a name too.
By the time you get done fixing the portability issue, I suspect you
won't have a union at all for the first case. I'm not real sure that
you need a union for the second case either --- is it really important
to shave a few bytes from the size of this struct? So you don't
necessarily need to do a notation change for the second union.
What drew my attention to it was an older gcc version complaining
thusly:
In file included from ../../../../src/include/replication/decode.h:13,
from decode.c:38:
../../../../src/include/replication/reorderbuffer.h:60: warning: unnamed
struct/union that defines no instances
../../../../src/include/replication/reorderbuffer.h:94: warning: unnamed
struct/union that defines no instances
decode.c: In function `DecodeInsert':
decode.c:588: structure has no member named `action'
decode.c:589: structure has no member named `tp'
decode.c:595: structure has no member named `tp'
decode.c:599: structure has no member named `tp'
decode.c: In function `DecodeUpdate':
decode.c:628: structure has no member named `action'
decode.c:629: structure has no member named `tp'
decode.c:637: structure has no member named `tp'
decode.c:641: structure has no member named `tp'
decode.c:651: structure has no member named `tp'
decode.c:654: structure has no member named `tp'
... etc etc ...
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
