Re: [HACKERS] WIP patch for Oid formatting in printf/elog strings
Mark Dilger wrote: I've been going through a copy of the code and replacing int32 and uint32 with the appropriate type such as Oid, TransactionId, and such, and have created my own typedef for TypeModType, in order to help chase through the code and figure out what functions handle what kind of data. By defining TypeModType to int64, for example, I get lots of compiler errors when passing a TypeModType * into a function that takes int32 *, and that lets me know that the callers of that function think of it as a typemod argument, even if it does not have any clear documentation to that effect. The exercise is a bit tedious, as I have to change lots of strings like the value %d is out of bounds to something like the value TYPEMODTYPE_FORMAT is out of bounds, but the clarity I get from it helps enough that it is useful to me. If it weren't for the format string issue, which makes translations impossible, I'd say let's go ahead with these ideas. For all the drawbacks that a 64-bit TransactionId has, I'm sure there's people who would prefer that to the constant vacuuming the 32-bit system causes. But the int32/TypModType thing looks like we could carry without causing any trouble at all. Not the format string part of it, though. How large is a patch that changes typmod from int32 to TypModType? I can see value in having 64-bit OIDs, for databases with large number of toasted items. Not in the default build, mind you. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] WIP patch for Oid formatting in printf/elog strings
I found a few places in the code where a variable of type Oid is printed using %d rather than %u and changed them in the attached patch. In src/backend/replication/logical/reorderbuffer.c, circa line 2494, chunk_seq is of type Oid, but ent-last_chunk_seq is of type int32, leading me to question if perhaps the use of %d for chunk_seq is correct, but the use of Oid for the type of chunk_seq is in error. If neither is in error, perhaps someone can provide a short code comment explaining the logic of the signed/unsigned discrepancy. Thanks, Mark Dilger diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c index ce44bbd..d230387 100644 --- a/src/backend/access/heap/tuptoaster.c +++ b/src/backend/access/heap/tuptoaster.c @@ -2155,7 +2155,7 @@ toast_open_indexes(Relation toastrel, * wrong if there is nothing. */ if (!found) - elog(ERROR, no valid index found for toast relation with Oid %d, + elog(ERROR, no valid index found for toast relation with Oid %u, RelationGetRelid(toastrel)); return res; diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index c7f41a5..c25ac31 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -4540,7 +4540,7 @@ pgstat_recv_inquiry(PgStat_MsgInquiry *msg, int len) DBWriteRequest *newreq; PgStat_StatDBEntry *dbentry; - elog(DEBUG2, received inquiry for %d, msg-databaseid); + elog(DEBUG2, received inquiry for %u, msg-databaseid); /* * Find the last write request for this DB. If it's older than the @@ -4598,7 +4598,7 @@ pgstat_recv_inquiry(PgStat_MsgInquiry *msg, int len) writetime = pstrdup(timestamptz_to_str(dbentry-stats_timestamp)); mytime = pstrdup(timestamptz_to_str(cur_ts)); elog(LOG, - stats_timestamp %s is later than collector's time %s for db %d, + stats_timestamp %s is later than collector's time %s for db %u, writetime, mytime, dbentry-databaseid); pfree(writetime); pfree(mytime); diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 6e75398..41a4896 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2487,11 +2487,11 @@ ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *txn, dlist_init(ent-chunks); if (chunk_seq != 0) - elog(ERROR, got sequence entry %d for toast chunk %u instead of seq 0, + elog(ERROR, got sequence entry %u for toast chunk %u instead of seq 0, chunk_seq, chunk_id); } else if (found chunk_seq != ent-last_chunk_seq + 1) - elog(ERROR, got sequence entry %d for toast chunk %u instead of seq %d, + elog(ERROR, got sequence entry %u for toast chunk %u instead of seq %d, chunk_seq, chunk_id, ent-last_chunk_seq + 1); chunk = DatumGetPointer(fastgetattr(newtup-tuple, 3, desc, isnull)); diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index 0a9ac02..a59508f 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -821,7 +821,7 @@ lockTableNoWait(ArchiveHandle *AH, TocEntry *te) pg_class.relname FROM pg_class JOIN pg_namespace on pg_namespace.oid = relnamespace - WHERE pg_class.oid = %d, te-catalogId.oid); + WHERE pg_class.oid = %u, te-catalogId.oid); res = PQexec(AH-connection, query-data); -- 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] WIP patch for Oid formatting in printf/elog strings
Mark Dilger wrote: I found a few places in the code where a variable of type Oid is printed using %d rather than %u and changed them in the attached patch. In src/backend/replication/logical/reorderbuffer.c, circa line 2494, chunk_seq is of type Oid, but ent-last_chunk_seq is of type int32, leading me to question if perhaps the use of %d for chunk_seq is correct, but the use of Oid for the type of chunk_seq is in error. If neither is in error, perhaps someone can provide a short code comment explaining the logic of the signed/unsigned discrepancy. tuptoaster defines chunk_seq as signed int32; ReorderBufferToastAppendChunk is wrong in declaring it Oid, and so this part of your patch is bogus. The others seem correct. diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 6e75398..41a4896 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2487,11 +2487,11 @@ ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *txn, dlist_init(ent-chunks); if (chunk_seq != 0) - elog(ERROR, got sequence entry %d for toast chunk %u instead of seq 0, + elog(ERROR, got sequence entry %u for toast chunk %u instead of seq 0, chunk_seq, chunk_id); } else if (found chunk_seq != ent-last_chunk_seq + 1) - elog(ERROR, got sequence entry %d for toast chunk %u instead of seq %d, + elog(ERROR, got sequence entry %u for toast chunk %u instead of seq %d, chunk_seq, chunk_id, ent-last_chunk_seq + 1); chunk = DatumGetPointer(fastgetattr(newtup-tuple, 3, desc, isnull)); -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] WIP patch for Oid formatting in printf/elog strings
Thank you Alvaro, The attached patch changes the type of chunk_seq to int32, rather than changing the %d formatting. The other changes are the same as in the previous patch. Mark Dilger On Thu, Dec 11, 2014 at 9:39 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Mark Dilger wrote: I found a few places in the code where a variable of type Oid is printed using %d rather than %u and changed them in the attached patch. In src/backend/replication/logical/reorderbuffer.c, circa line 2494, chunk_seq is of type Oid, but ent-last_chunk_seq is of type int32, leading me to question if perhaps the use of %d for chunk_seq is correct, but the use of Oid for the type of chunk_seq is in error. If neither is in error, perhaps someone can provide a short code comment explaining the logic of the signed/unsigned discrepancy. tuptoaster defines chunk_seq as signed int32; ReorderBufferToastAppendChunk is wrong in declaring it Oid, and so this part of your patch is bogus. The others seem correct. diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 6e75398..41a4896 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2487,11 +2487,11 @@ ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *txn, dlist_init(ent-chunks); if (chunk_seq != 0) - elog(ERROR, got sequence entry %d for toast chunk %u instead of seq 0, + elog(ERROR, got sequence entry %u for toast chunk %u instead of seq 0, chunk_seq, chunk_id); } else if (found chunk_seq != ent-last_chunk_seq + 1) - elog(ERROR, got sequence entry %d for toast chunk %u instead of seq %d, + elog(ERROR, got sequence entry %u for toast chunk %u instead of seq %d, chunk_seq, chunk_id, ent-last_chunk_seq + 1); chunk = DatumGetPointer(fastgetattr(newtup-tuple, 3, desc, isnull)); -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c index ce44bbd..d230387 100644 --- a/src/backend/access/heap/tuptoaster.c +++ b/src/backend/access/heap/tuptoaster.c @@ -2155,7 +2155,7 @@ toast_open_indexes(Relation toastrel, * wrong if there is nothing. */ if (!found) - elog(ERROR, no valid index found for toast relation with Oid %d, + elog(ERROR, no valid index found for toast relation with Oid %u, RelationGetRelid(toastrel)); return res; diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index c7f41a5..c25ac31 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -4540,7 +4540,7 @@ pgstat_recv_inquiry(PgStat_MsgInquiry *msg, int len) DBWriteRequest *newreq; PgStat_StatDBEntry *dbentry; - elog(DEBUG2, received inquiry for %d, msg-databaseid); + elog(DEBUG2, received inquiry for %u, msg-databaseid); /* * Find the last write request for this DB. If it's older than the @@ -4598,7 +4598,7 @@ pgstat_recv_inquiry(PgStat_MsgInquiry *msg, int len) writetime = pstrdup(timestamptz_to_str(dbentry-stats_timestamp)); mytime = pstrdup(timestamptz_to_str(cur_ts)); elog(LOG, - stats_timestamp %s is later than collector's time %s for db %d, + stats_timestamp %s is later than collector's time %s for db %u, writetime, mytime, dbentry-databaseid); pfree(writetime); pfree(mytime); diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 6e75398..cd132c1 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2458,7 +2458,7 @@ ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *txn, Pointer chunk; TupleDesc desc = RelationGetDescr(relation); Oid chunk_id; - Oid chunk_seq; + int32 chunk_seq; if (txn-toast_hash == NULL) ReorderBufferToastInitHash(rb, txn); diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index 0a9ac02..a59508f 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -821,7 +821,7 @@ lockTableNoWait(ArchiveHandle *AH, TocEntry *te) pg_class.relname FROM pg_class
Re: [HACKERS] WIP patch for Oid formatting in printf/elog strings
Mark Dilger hornschnor...@gmail.com writes: The attached patch changes the type of chunk_seq to int32, rather than changing the %d formatting. The other changes are the same as in the previous patch. BTW, how are you finding these? If it's some automated tool, what? (If you're finding them by hand, I'm in awe of your scan rate.) 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] WIP patch for Oid formatting in printf/elog strings
Tom Lane wrote: Mark Dilger hornschnor...@gmail.com writes: The attached patch changes the type of chunk_seq to int32, rather than changing the %d formatting. The other changes are the same as in the previous patch. BTW, how are you finding these? If it's some automated tool, what? (If you're finding them by hand, I'm in awe of your scan rate.) BTW if it's an automated tool, it might also improve by examining the DatumGetFoo() macros. In the reorderbuffer.c code just fixed you can see that chunk_seq is obtained by DatumGetInt32(), which was wrong to assign to a value of type Oid. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] WIP patch for Oid formatting in printf/elog strings
On Thu, Dec 11, 2014 at 12:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Mark Dilger hornschnor...@gmail.com writes: The attached patch changes the type of chunk_seq to int32, rather than changing the %d formatting. The other changes are the same as in the previous patch. BTW, how are you finding these? If it's some automated tool, what? (If you're finding them by hand, I'm in awe of your scan rate.) regards, tom lane I've been advised to hoodwink you and say that I have found them all by hand. Actually, I am changing the typedef in src/include/c.h and then recompiling, and watching for compiler warnings telling me that I am passing a uint64 to a %d. Of course, I get lots of warnings about passing a uint64 to a %u, but I can filter through those easily enough. Also, the macros in outfuncs.c tend to complain in a similar way, and I can check that output, too. mark
Re: [HACKERS] WIP patch for Oid formatting in printf/elog strings
I spoke too soon. Actually, the Oid definition is in src/include/postgres_ext.h, which I'm sure you all know. But I'm changing other typedefs too, not just for Oid. I've been going through a copy of the code and replacing int32 and uint32 with the appropriate type such as Oid, TransactionId, and such, and have created my own typedef for TypeModType, in order to help chase through the code and figure out what functions handle what kind of data. By defining TypeModType to int64, for example, I get lots of compiler errors when passing a TypeModType * into a function that takes int32 *, and that lets me know that the callers of that function think of it as a typemod argument, even if it does not have any clear documentation to that effect. The exercise is a bit tedious, as I have to change lots of strings like the value %d is out of bounds to something like the value TYPEMODTYPE_FORMAT is out of bounds, but the clarity I get from it helps enough that it is useful to me. I hope to eventually be able to simply pass switches to configure like --64bit-oids and have the whole system work with Oid defined as 64-bits. Likewise for varlena headers. I ported the whole of postgres to 64-bit Oids and 64-bit varlena headers once before, and it worked and passed all the regression tests and such, but I did it by replacing lots of instances of int32 with instances of int64, so it didn't help clarify the meaning of anything. This time, I'm hoping that I can keep in sync with all the commits so that I can build a 32-bit or a 64-bit postgres at a moments notice. Mark Dilger On Thu, Dec 11, 2014 at 1:25 PM, Mark Dilger hornschnor...@gmail.com wrote: On Thu, Dec 11, 2014 at 12:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Mark Dilger hornschnor...@gmail.com writes: The attached patch changes the type of chunk_seq to int32, rather than changing the %d formatting. The other changes are the same as in the previous patch. BTW, how are you finding these? If it's some automated tool, what? (If you're finding them by hand, I'm in awe of your scan rate.) regards, tom lane I've been advised to hoodwink you and say that I have found them all by hand. Actually, I am changing the typedef in src/include/c.h and then recompiling, and watching for compiler warnings telling me that I am passing a uint64 to a %d. Of course, I get lots of warnings about passing a uint64 to a %u, but I can filter through those easily enough. Also, the macros in outfuncs.c tend to complain in a similar way, and I can check that output, too. mark
Re: [HACKERS] WIP patch for Oid formatting in printf/elog strings
On Thu, Dec 11, 2014 at 01:25:00PM -0800, Mark Dilger wrote: On Thu, Dec 11, 2014 at 12:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Mark Dilger hornschnor...@gmail.com writes: The attached patch changes the type of chunk_seq to int32, rather than changing the %d formatting. The other changes are the same as in the previous patch. BTW, how are you finding these? If it's some automated tool, what? (If you're finding them by hand, I'm in awe of your scan rate.) regards, tom lane I've been advised to hoodwink you and say that I have found them all by hand. Actually, I am changing the typedef in src/include/c.h and then recompiling, and watching for compiler warnings telling me that I am passing a uint64 to a %d. Of course, I get lots of warnings about passing a uint64 to a %u, but I can filter through those easily enough. Cool idea! -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
