Re: Using defines for protocol characters
On Thu, Aug 17, 2023 at 09:52:03AM -0700, Nathan Bossart wrote: > On Thu, Aug 17, 2023 at 12:22:24PM -0400, Robert Haas wrote: >> I think this is going to be a major improvement in terms of readability! >> >> I'm pretty happy with this version, too. We may be running out of >> things to argue about -- and no, I don't want to argue about >> underscores more! > > Awesome. If we are indeed out of things to argue about, I'll plan on > committing this sometime next week. Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Using defines for protocol characters
On Thu, Aug 17, 2023 at 12:22:24PM -0400, Robert Haas wrote: > I think this is going to be a major improvement in terms of readability! > > I'm pretty happy with this version, too. We may be running out of > things to argue about -- and no, I don't want to argue about > underscores more! Awesome. If we are indeed out of things to argue about, I'll plan on committing this sometime next week. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Using defines for protocol characters
On Thu, Aug 17, 2023 at 9:22 AM Robert Haas wrote: > On Thu, Aug 17, 2023 at 12:13 PM Nathan Bossart > wrote: > > On Thu, Aug 17, 2023 at 09:31:55AM +0900, Michael Paquier wrote: > > > Looks sensible seen from here. > > > > Thanks for taking a look. > > I think this is going to be a major improvement in terms of readability! That was the primary goal > Dave > > -- Dave Cramer
Re: Using defines for protocol characters
On Thu, Aug 17, 2023 at 12:13 PM Nathan Bossart wrote: > On Thu, Aug 17, 2023 at 09:31:55AM +0900, Michael Paquier wrote: > > Looks sensible seen from here. > > Thanks for taking a look. I think this is going to be a major improvement in terms of readability! I'm pretty happy with this version, too. We may be running out of things to argue about -- and no, I don't want to argue about underscores more! -- Robert Haas EDB: http://www.enterprisedb.com
Re: Using defines for protocol characters
On Thu, Aug 17, 2023 at 09:31:55AM +0900, Michael Paquier wrote:
> Looks sensible seen from here.
Thanks for taking a look.
> This patch is missing the installation of protocol.h in
> src/tools/msvc/Install.pm for MSVC. For pqcomm.h, we are doing that:
> lcopy('src/include/libpq/pqcomm.h', $target . '/include/internal/libpq/')
> || croak 'Could not copy pqcomm.h';
>
> So adding two similar lines for protocol.h should be enough (I assume,
> did not test).
I added those lines in v7.
> In fe-exec.c, we still have a few things for the type of objects to
> work on:
> - 'S' for statement.
> - 'P' for portal.
> Should these be added to protocol.h? They are part of the extended
> protocol.
IMHO they should be added, but I've intentionally restricted this first
patch to only codes with existing names in protocol.sgml. I figured we
could work on naming other things in a follow-up discussion.
> The comment at the top of PQsendTypedCommand() mentions 'C' and 'D',
> but perhaps these should be updated to the object names instead?
Done.
> pqFunctionCall3(), for PQfn(), has a few more hardcoded characters for
> its status codes. I'm OK to do things incrementally so it's fine by
> me to not add them now, just noticing on the way what could be added
> to this new header.
Cool, thanks.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 69f94689857a469333fecbfd2057868140796516 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 17 Aug 2023 09:00:46 -0700
Subject: [PATCH v7 1/1] Introduce macros for protocol characters.
Author: Dave Cramer
Reviewed-by: Alvaro Herrera, Tatsuo Ishii, Peter Smith, Robert Haas, Tom Lane, Peter Eisentraut, Michael Paquier
Discussion: https://postgr.es/m/CADK3HHKbBmK-PKf1bPNFoMC%2BoBt%2BpD9PH8h5nvmBQskEHm-Ehw%40mail.gmail.com
---
src/backend/access/common/printsimple.c | 5 +-
src/backend/access/transam/parallel.c | 14 ++--
src/backend/backup/basebackup_copy.c| 16 ++---
src/backend/commands/async.c| 2 +-
src/backend/commands/copyfromparse.c| 22 +++
src/backend/commands/copyto.c | 6 +-
src/backend/libpq/auth-sasl.c | 2 +-
src/backend/libpq/auth.c| 8 +--
src/backend/postmaster/postmaster.c | 2 +-
src/backend/replication/walsender.c | 18 +++---
src/backend/tcop/dest.c | 8 +--
src/backend/tcop/fastpath.c | 2 +-
src/backend/tcop/postgres.c | 68 ++--
src/backend/utils/error/elog.c | 5 +-
src/backend/utils/misc/guc.c| 2 +-
src/include/Makefile| 3 +-
src/include/libpq/pqcomm.h | 23 ++-
src/include/libpq/protocol.h| 85 +
src/include/meson.build | 1 +
src/interfaces/libpq/fe-auth.c | 2 +-
src/interfaces/libpq/fe-connect.c | 19 --
src/interfaces/libpq/fe-exec.c | 54
src/interfaces/libpq/fe-protocol3.c | 70 ++--
src/interfaces/libpq/fe-trace.c | 70 +++-
src/tools/msvc/Install.pm | 2 +
25 files changed, 305 insertions(+), 204 deletions(-)
create mode 100644 src/include/libpq/protocol.h
diff --git a/src/backend/access/common/printsimple.c b/src/backend/access/common/printsimple.c
index ef818228ac..675b744db2 100644
--- a/src/backend/access/common/printsimple.c
+++ b/src/backend/access/common/printsimple.c
@@ -20,6 +20,7 @@
#include "access/printsimple.h"
#include "catalog/pg_type.h"
+#include "libpq/protocol.h"
#include "libpq/pqformat.h"
#include "utils/builtins.h"
@@ -32,7 +33,7 @@ printsimple_startup(DestReceiver *self, int operation, TupleDesc tupdesc)
StringInfoData buf;
int i;
- pq_beginmessage(&buf, 'T'); /* RowDescription */
+ pq_beginmessage(&buf, PqMsg_RowDescription);
pq_sendint16(&buf, tupdesc->natts);
for (i = 0; i < tupdesc->natts; ++i)
@@ -65,7 +66,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)
slot_getallattrs(slot);
/* Prepare and send message */
- pq_beginmessage(&buf, 'D');
+ pq_beginmessage(&buf, PqMsg_DataRow);
pq_sendint16(&buf, tupdesc->natts);
for (i = 0; i < tupdesc->natts; ++i)
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 1738aecf1f..194a1207be 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1127,7 +1127,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
switch (msgtype)
{
- case 'K':/* BackendKeyData */
+ case PqMsg_BackendKeyData:
{
int32 pid = pq_getmsgint(msg, 4);
@@ -1137,8 +1137,8 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
break;
}
- case 'E':/* ErrorResponse */
- case 'N':/* NoticeResponse */
+ case PqMsg_ErrorResponse:
+ case PqMsg_NoticeResponse:
{
ErrorData edata;
ErrorContextCallback *save_error_context_s
Re: Using defines for protocol characters
On Wed, Aug 16, 2023 at 12:29:56PM -0700, Nathan Bossart wrote:
> I moved the definitions out to a separate file in v6.
Looks sensible seen from here.
This patch is missing the installation of protocol.h in
src/tools/msvc/Install.pm for MSVC. For pqcomm.h, we are doing that:
lcopy('src/include/libpq/pqcomm.h', $target . '/include/internal/libpq/')
|| croak 'Could not copy pqcomm.h';
So adding two similar lines for protocol.h should be enough (I assume,
did not test).
In fe-exec.c, we still have a few things for the type of objects to
work on:
- 'S' for statement.
- 'P' for portal.
Should these be added to protocol.h? They are part of the extended
protocol.
The comment at the top of PQsendTypedCommand() mentions 'C' and 'D',
but perhaps these should be updated to the object names instead?
pqFunctionCall3(), for PQfn(), has a few more hardcoded characters for
its status codes. I'm OK to do things incrementally so it's fine by
me to not add them now, just noticing on the way what could be added
to this new header.
--
Michael
signature.asc
Description: PGP signature
Re: Using defines for protocol characters
On Tue, Aug 15, 2023 at 11:40:07PM +0200, Alvaro Herrera wrote:
> On 2023-Aug-16, Michael Paquier wrote:
>
>> On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote:
>> > Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
>> > what about other middleware?
>>
>> Why do you need to include directly c.h? There are definitions in
>> there that are not intended to be exposed.
>
> What this argument says is that these new defines should be in a
> separate file, not in pqcomm.h. IMO that makes sense, precisely because
> these defines should be usable by third parties.
I moved the definitions out to a separate file in v6.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From fc54d1655fe662abe99a4605bdff2d1b65f0dc2a Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 16 Aug 2023 12:08:29 -0700
Subject: [PATCH v6 1/1] Introduce macros for protocol characters.
Author: Dave Cramer
Reviewed-by: Alvaro Herrera, Tatsuo Ishii, Peter Smith, Robert Haas, Tom Lane, Peter Eisentraut
Discussion: https://postgr.es/m/CADK3HHKbBmK-PKf1bPNFoMC%2BoBt%2BpD9PH8h5nvmBQskEHm-Ehw%40mail.gmail.com
---
src/backend/access/common/printsimple.c | 5 +-
src/backend/access/transam/parallel.c | 14 ++--
src/backend/backup/basebackup_copy.c| 16 ++---
src/backend/commands/async.c| 2 +-
src/backend/commands/copyfromparse.c| 22 +++
src/backend/commands/copyto.c | 6 +-
src/backend/libpq/auth-sasl.c | 2 +-
src/backend/libpq/auth.c| 8 +--
src/backend/postmaster/postmaster.c | 2 +-
src/backend/replication/walsender.c | 18 +++---
src/backend/tcop/dest.c | 8 +--
src/backend/tcop/fastpath.c | 2 +-
src/backend/tcop/postgres.c | 68 ++--
src/backend/utils/error/elog.c | 5 +-
src/backend/utils/misc/guc.c| 2 +-
src/include/Makefile| 3 +-
src/include/libpq/pqcomm.h | 23 ++-
src/include/libpq/protocol.h| 85 +
src/include/meson.build | 1 +
src/interfaces/libpq/fe-auth.c | 2 +-
src/interfaces/libpq/fe-connect.c | 19 --
src/interfaces/libpq/fe-exec.c | 50 +++
src/interfaces/libpq/fe-protocol3.c | 70 ++--
src/interfaces/libpq/fe-trace.c | 70 +++-
24 files changed, 301 insertions(+), 202 deletions(-)
create mode 100644 src/include/libpq/protocol.h
diff --git a/src/backend/access/common/printsimple.c b/src/backend/access/common/printsimple.c
index ef818228ac..675b744db2 100644
--- a/src/backend/access/common/printsimple.c
+++ b/src/backend/access/common/printsimple.c
@@ -20,6 +20,7 @@
#include "access/printsimple.h"
#include "catalog/pg_type.h"
+#include "libpq/protocol.h"
#include "libpq/pqformat.h"
#include "utils/builtins.h"
@@ -32,7 +33,7 @@ printsimple_startup(DestReceiver *self, int operation, TupleDesc tupdesc)
StringInfoData buf;
int i;
- pq_beginmessage(&buf, 'T'); /* RowDescription */
+ pq_beginmessage(&buf, PqMsg_RowDescription);
pq_sendint16(&buf, tupdesc->natts);
for (i = 0; i < tupdesc->natts; ++i)
@@ -65,7 +66,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)
slot_getallattrs(slot);
/* Prepare and send message */
- pq_beginmessage(&buf, 'D');
+ pq_beginmessage(&buf, PqMsg_DataRow);
pq_sendint16(&buf, tupdesc->natts);
for (i = 0; i < tupdesc->natts; ++i)
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 1738aecf1f..194a1207be 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1127,7 +1127,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
switch (msgtype)
{
- case 'K':/* BackendKeyData */
+ case PqMsg_BackendKeyData:
{
int32 pid = pq_getmsgint(msg, 4);
@@ -1137,8 +1137,8 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
break;
}
- case 'E':/* ErrorResponse */
- case 'N':/* NoticeResponse */
+ case PqMsg_ErrorResponse:
+ case PqMsg_NoticeResponse:
{
ErrorData edata;
ErrorContextCallback *save_error_context_stack;
@@ -1183,7 +1183,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
break;
}
- case 'A':/* NotifyResponse */
+ case PqMsg_NotificationResponse:
{
/* Propagate NotifyResponse. */
int32 pid;
@@ -1217,7 +1217,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
break;
}
- case 'X':/* Terminate, indicating clean exit */
+ case PqMsg_Terminate:
{
shm_mq_detach(pcxt->worker[i].error_mqh);
pcxt->worker[i].error_mqh = NULL;
@@ -1372,7 +1372,7 @@ ParallelWorkerMain(Datum main_arg)
* protocol message is defined, but it won't actually be used for anything
* in this case.
*/
- pq_beginmessage(&msgbuf, 'K')
Re: Using defines for protocol characters
> On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote: >> Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But >> what about other middleware? > > Why do you need to include directly c.h? There are definitions in > there that are not intended to be exposed. pqcomm.h has this: #define UNIXSOCK_PATH(path, port, sockdir) \ (AssertMacro(sockdir), \ AssertMacro(*(sockdir) != '\0'), \ snprintf(path, sizeof(path), "%s/.s.PGSQL.%d", \ (sockdir), (port))) AssertMacro is defined in c.h. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
On 2023-Aug-16, Michael Paquier wrote: > On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote: > > Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But > > what about other middleware? > > Why do you need to include directly c.h? There are definitions in > there that are not intended to be exposed. What this argument says is that these new defines should be in a separate file, not in pqcomm.h. IMO that makes sense, precisely because these defines should be usable by third parties. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Using defines for protocol characters
On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote: > Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But > what about other middleware? Why do you need to include directly c.h? There are definitions in there that are not intended to be exposed. -- Michael signature.asc Description: PGP signature
Re: Using defines for protocol characters
> On Tue, Aug 15, 2023 at 02:46:24PM +0900, Tatsuo Ishii wrote: >> Is it possible to put the new define staff into protocol.h then let >> pqcomm.h include protocol.h? This makes Pgpool-II and other middle >> ware/drivers written in C easier to use the defines so that they only >> include protocol.h to use the defines. > > It is possible, of course, but are there any reasons such programs couldn't > include pqcomm.h? It looks relatively inexpensive to me. That being said, > I'm fine with the approach you describe if the folks in this thread agree. Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But what about other middleware? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
On Tue, Aug 15, 2023 at 02:46:24PM +0900, Tatsuo Ishii wrote: > Is it possible to put the new define staff into protocol.h then let > pqcomm.h include protocol.h? This makes Pgpool-II and other middle > ware/drivers written in C easier to use the defines so that they only > include protocol.h to use the defines. It is possible, of course, but are there any reasons such programs couldn't include pqcomm.h? It looks relatively inexpensive to me. That being said, I'm fine with the approach you describe if the folks in this thread agree. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Using defines for protocol characters
> I tried to address all the feedback in v5 of the patch, which is attached.
> I limited the patch to only the characters that have names in the "Message
> Formats" section of protocol.sgml instead of trying to invent names for
> unnamed characters.
>
> I'm aware of one inconsistency. While I grouped all the authentication
> request messages ('R') into PqMsg_AuthenticationRequest, I added separate
> macros for all the different meanings of 'p', i.e., GSSResponse,
> PasswordMessage, SASLInitialResponse, and SASLResponse. I wanted to list
> everything in protocol.sgml (even the duplicate ) to ease greppability, but
> the code is structure such that adding macros for all the different
> authentication messages would be kind of pointless. Plus, there is a
> separate set of authentication request codes just below the added macros.
> So, this is where I landed...
Is it possible to put the new define staff into protocol.h then let
pqcomm.h include protocol.h? This makes Pgpool-II and other middle
ware/drivers written in C easier to use the defines so that they only
include protocol.h to use the defines.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
I tried to address all the feedback in v5 of the patch, which is attached.
I limited the patch to only the characters that have names in the "Message
Formats" section of protocol.sgml instead of trying to invent names for
unnamed characters.
I'm aware of one inconsistency. While I grouped all the authentication
request messages ('R') into PqMsg_AuthenticationRequest, I added separate
macros for all the different meanings of 'p', i.e., GSSResponse,
PasswordMessage, SASLInitialResponse, and SASLResponse. I wanted to list
everything in protocol.sgml (even the duplicateѕ) to ease greppability, but
the code is structure such that adding macros for all the different
authentication messages would be kind of pointless. Plus, there is a
separate set of authentication request codes just below the added macros.
So, this is where I landed...
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 78c2a110a8c940690d1ff363e545acb48adf966e Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Mon, 14 Aug 2023 14:09:30 -0700
Subject: [PATCH v5 1/1] Introduce macros for protocol characters.
Author: Dave Cramer
Reviewed-by: Alvaro Herrera, Tatsuo Ishii, Peter Smith, Robert Haas, Tom Lane, Peter Eisentraut
Discussion: https://postgr.es/m/CADK3HHKbBmK-PKf1bPNFoMC%2BoBt%2BpD9PH8h5nvmBQskEHm-Ehw%40mail.gmail.com
---
src/backend/access/common/printsimple.c | 5 +-
src/backend/access/transam/parallel.c | 14 ++---
src/backend/backup/basebackup_copy.c| 16 +++---
src/backend/commands/async.c| 2 +-
src/backend/commands/copyfromparse.c| 22
src/backend/commands/copyto.c | 6 +--
src/backend/libpq/auth-sasl.c | 2 +-
src/backend/libpq/auth.c| 8 +--
src/backend/postmaster/postmaster.c | 2 +-
src/backend/replication/walsender.c | 18 +++
src/backend/tcop/dest.c | 8 +--
src/backend/tcop/fastpath.c | 2 +-
src/backend/tcop/postgres.c | 68
src/backend/utils/error/elog.c | 5 +-
src/backend/utils/misc/guc.c| 2 +-
src/include/libpq/pqcomm.h | 51 ++
src/interfaces/libpq/fe-auth.c | 2 +-
src/interfaces/libpq/fe-connect.c | 19 ---
src/interfaces/libpq/fe-exec.c | 50 +-
src/interfaces/libpq/fe-protocol3.c | 70 +
src/interfaces/libpq/fe-trace.c | 70 ++---
21 files changed, 258 insertions(+), 184 deletions(-)
diff --git a/src/backend/access/common/printsimple.c b/src/backend/access/common/printsimple.c
index ef818228ac..62de95e1ba 100644
--- a/src/backend/access/common/printsimple.c
+++ b/src/backend/access/common/printsimple.c
@@ -20,6 +20,7 @@
#include "access/printsimple.h"
#include "catalog/pg_type.h"
+#include "libpq/pqcomm.h"
#include "libpq/pqformat.h"
#include "utils/builtins.h"
@@ -32,7 +33,7 @@ printsimple_startup(DestReceiver *self, int operation, TupleDesc tupdesc)
StringInfoData buf;
int i;
- pq_beginmessage(&buf, 'T'); /* RowDescription */
+ pq_beginmessage(&buf, PqMsg_RowDescription);
pq_sendint16(&buf, tupdesc->natts);
for (i = 0; i < tupdesc->natts; ++i)
@@ -65,7 +66,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)
slot_getallattrs(slot);
/* Prepare and send message */
- pq_beginmessage(&buf, 'D');
+ pq_beginmessage(&buf, PqMsg_DataRow);
pq_sendint16(&buf, tupdesc->natts);
for (i = 0; i < tupdesc->natts; ++i)
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 1738aecf1f..194a1207be 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1127,7 +1127,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
switch (msgtype)
{
- case 'K':/* BackendKeyData */
+ case PqMsg_BackendKeyData:
{
int32 pid = pq_getmsgint(msg, 4);
@@ -1137,8 +1137,8 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
break;
}
- case 'E':/* ErrorResponse */
- case 'N':/* NoticeResponse */
+ case PqMsg_ErrorResponse:
+ case PqMsg_NoticeResponse:
{
ErrorData edata;
ErrorContextCallback *save_error_context_stack;
@@ -1183,7 +1183,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
break;
}
- case 'A':/* NotifyResponse */
+ case PqMsg_NotificationResponse:
{
/* Propagate NotifyResponse. */
int32 pid;
@@ -1217,7 +1217,7 @@ HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg)
break;
}
- case 'X':/* Terminate, indicating clean exit */
+ case PqMsg_Terminate:
{
shm_mq_detach(pcxt->worker[i].error_mqh);
pcxt->worker[i].error_mqh = NULL;
@@ -1372,7 +1372,7 @@ ParallelWorkerMain(Datum main_arg)
* protocol message is defined, but it won't actually be used for anything
* in this case.
*/
- pq_beginmessage(&msgb
Re: Using defines for protocol characters
On 2023-Aug-09, Nathan Bossart wrote: > On Wed, Aug 09, 2023 at 10:44:42AM -0600, Dave Cramer wrote: > > On Wed, 9 Aug 2023 at 10:34, Tom Lane wrote: > >> I agree with Peter: let's use the names in the protocol document > >> with a single prefix. I've got mixed feelings about whether that prefix > >> should have an underscore, though. > > > > Well, we're getting closer :) > > I'm +0.5 for the underscore. We use CamelCase_With_UnderScores in other places (PgStat_Counter, AtEOXact_PgStat_Database). It's not pretty, and at times it's not comfortable to type, but I think it will be less ugly here than in those other places (particularly because there'll be a single underscore), and I also agree that readability is better with the underscore than without. So, I'm also +0.5 on having them, much as it displeases Robert. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Using defines for protocol characters
On Wed, Aug 09, 2023 at 10:44:42AM -0600, Dave Cramer wrote: > On Wed, 9 Aug 2023 at 10:34, Tom Lane wrote: >> I agree with Peter: let's use the names in the protocol document >> with a single prefix. I've got mixed feelings about whether that prefix >> should have an underscore, though. > > Well, we're getting closer :) I'm +0.5 for the underscore. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Using defines for protocol characters
On Wed, 9 Aug 2023 at 10:34, Tom Lane wrote: > Dave Cramer writes: > > On Wed, 9 Aug 2023 at 09:19, Peter Eisentraut > wrote: > >> 3. IMO, the names of the protocol messages in protocol.sgml are > >> canonical. Your patch appends "Request" and "Response" in cases where > >> that is not part of the actual name. Also, some messages are documented > >> to go both ways, so this separation doesn't make sense strictly > >> speaking. Please use the names as in protocol.sgml without augmenting > >> them. > > > I've changed this a number of times. I do not mind changing it again, but > > can we reach a consensus ? > > I agree with Peter: let's use the names in the protocol document > with a single prefix. I've got mixed feelings about whether that prefix > should have an underscore, though. > Well, we're getting closer :) Dave
Re: Using defines for protocol characters
Dave Cramer writes: > On Wed, 9 Aug 2023 at 09:19, Peter Eisentraut wrote: >> 3. IMO, the names of the protocol messages in protocol.sgml are >> canonical. Your patch appends "Request" and "Response" in cases where >> that is not part of the actual name. Also, some messages are documented >> to go both ways, so this separation doesn't make sense strictly >> speaking. Please use the names as in protocol.sgml without augmenting >> them. > I've changed this a number of times. I do not mind changing it again, but > can we reach a consensus ? I agree with Peter: let's use the names in the protocol document with a single prefix. I've got mixed feelings about whether that prefix should have an underscore, though. regards, tom lane
Re: Using defines for protocol characters
On Wed, 9 Aug 2023 at 09:19, Peter Eisentraut wrote: > 1. As was discussed, these definitions should go into > src/include/libpq/pqcomm.h, not a new file. > I'm ambivalent, this is very easy to do. > > 2. I would prefer an underscore after PgMsg, like PqMsg_DescribeRequest, > so it's easier to visually locate the start of the actual message name. > > 3. IMO, the names of the protocol messages in protocol.sgml are > canonical. Your patch appends "Request" and "Response" in cases where > that is not part of the actual name. Also, some messages are documented > to go both ways, so this separation doesn't make sense strictly > speaking. Please use the names as in protocol.sgml without augmenting > them. > > I've changed this a number of times. I do not mind changing it again, but can we reach a consensus ? Dave
Re: Using defines for protocol characters
1. As was discussed, these definitions should go into src/include/libpq/pqcomm.h, not a new file. 2. I would prefer an underscore after PgMsg, like PqMsg_DescribeRequest, so it's easier to visually locate the start of the actual message name. 3. IMO, the names of the protocol messages in protocol.sgml are canonical. Your patch appends "Request" and "Response" in cases where that is not part of the actual name. Also, some messages are documented to go both ways, so this separation doesn't make sense strictly speaking. Please use the names as in protocol.sgml without augmenting them.
Re: Using defines for protocol characters
On 2023-Aug-07, Nathan Bossart wrote: > On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote: > > Dave Cramer writes: > >> Can we vote or whatever it takes to decide on a naming pattern that is > >> acceptable ? > > > > I'm good with Robert's proposal above. > > +1 WFM as well. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Every machine is a smoke machine if you operate it wrong enough." https://twitter.com/libseybieda/status/1541673325781196801
Re: Using defines for protocol characters
> On Mon, 7 Aug 2023 at 16:50, Tatsuo Ishii wrote: > >> > On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote: >> >> Dave Cramer writes: >> >>> On Mon, 7 Aug 2023 at 12:59, Robert Haas >> wrote: >> PqMsgEmptyQueryResponse or something like that seems better, if we >> want to keep the current capitalization. I'm not a huge fan of the way >> we vary our capitalization conventions so much all over the code base, >> but I think we would at least do well to keep it consistent from one >> end of a certain identifier to the other. >> >> >> >>> I don't have a strong preference, but before I make the changes I'd >> like to >> >>> get consensus. >> >>> Can we vote or whatever it takes to decide on a naming pattern that is >> >>> acceptable ? >> >> >> >> I'm good with Robert's proposal above. >> > >> > +1 >> >> +1. >> >> Also we need to decide what to do with them: >> >> > #define PQMSG_REQ_PREPARED 'S' >> > #define PQMSG_REQ_PORTAL 'P' >> >> If we go "PqMsgEmptyQueryResponse", probably we should go something >> like for these? >> >> #define PqMsgReqPrepared 'S' >> #define PqMsgReqPortal 'P' >> > > I went with PqMsgPortalSubCommand and PqMsgPreparedSubCommand > > See attached patch Looks good to me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
On Mon, 7 Aug 2023 at 16:50, Tatsuo Ishii wrote: > > On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote: > >> Dave Cramer writes: > >>> On Mon, 7 Aug 2023 at 12:59, Robert Haas > wrote: > PqMsgEmptyQueryResponse or something like that seems better, if we > want to keep the current capitalization. I'm not a huge fan of the way > we vary our capitalization conventions so much all over the code base, > but I think we would at least do well to keep it consistent from one > end of a certain identifier to the other. > >> > >>> I don't have a strong preference, but before I make the changes I'd > like to > >>> get consensus. > >>> Can we vote or whatever it takes to decide on a naming pattern that is > >>> acceptable ? > >> > >> I'm good with Robert's proposal above. > > > > +1 > > +1. > > Also we need to decide what to do with them: > > > #define PQMSG_REQ_PREPARED 'S' > > #define PQMSG_REQ_PORTAL 'P' > > If we go "PqMsgEmptyQueryResponse", probably we should go something > like for these? > > #define PqMsgReqPrepared 'S' > #define PqMsgReqPortal 'P' > I went with PqMsgPortalSubCommand and PqMsgPreparedSubCommand See attached patch Dave 0001-Created-protocol.h.patch Description: Binary data
Re: Using defines for protocol characters
> On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote: >> Dave Cramer writes: >>> On Mon, 7 Aug 2023 at 12:59, Robert Haas wrote: PqMsgEmptyQueryResponse or something like that seems better, if we want to keep the current capitalization. I'm not a huge fan of the way we vary our capitalization conventions so much all over the code base, but I think we would at least do well to keep it consistent from one end of a certain identifier to the other. >> >>> I don't have a strong preference, but before I make the changes I'd like to >>> get consensus. >>> Can we vote or whatever it takes to decide on a naming pattern that is >>> acceptable ? >> >> I'm good with Robert's proposal above. > > +1 +1. Also we need to decide what to do with them: > #define PQMSG_REQ_PREPARED 'S' > #define PQMSG_REQ_PORTAL 'P' If we go "PqMsgEmptyQueryResponse", probably we should go something like for these? #define PqMsgReqPrepared 'S' #define PqMsgReqPortal 'P' Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
+#ifndef _PROTOCOL_H +#define _PROTOCOL_H + +#define PQMSG_REQ_BIND 'B' +#define PQMSG_REQ_CLOSE 'C' +#define PQMSG_REQ_DESCRIBE 'D' +#define PQMSG_REQ_EXECUTE 'E' +#define PQMSG_REQ_FUNCTION_CALL 'F' +#define PQMSG_REQ_FLUSH_DATA'H' +#define PQMSG_REQ_BACKEND_KEY_DATA 'K' +#define PQMSG_REQ_PARSE 'P' +#define PQMSG_REQ_AUTHENTICATION'R' +#define PQMSG_REQ_SYNC_DATA 'S' +#define PQMSG_REQ_SIMPLE_QUERY 'Q' +#define PQMSG_REQ_TERMINATE 'X' +#define PQMSG_REQ_COPY_FAIL 'f' +#define PQMSG_REQ_COPY_DONE 'c' +#define PQMSG_REQ_COPY_DATA 'd' +#define PQMSG_REQ_COPY_PROGRESS 'p' +#define PQMSG_REQ_PREPARED 'S' +#define PQMSG_REQ_PORTAL'P' + + +/* +Responses +*/ +#define PQMSG_RESP_PARSE_COMPLETE '1' +#define PQMSG_RESP_BIND_COMPLETE'2' +#define PQMSG_RESP_CLOSE_COMPLETE '3' +#define PQMSG_RESP_NOTIFY 'A' +#define PQMSG_RESP_COMMAND_COMPLETE 'C' +#define PQMSG_RESP_DATA_ROW 'D' +#define PQMSG_RESP_ERROR'E' +#define PQMSG_RESP_COPY_IN 'G' +#define PQMSG_RESP_COPY_OUT 'H' +#define PQMSG_RESP_EMPTY_QUERY 'I' +#define PQMSG_RESP_NOTICE 'N' +#define PQMSG_RESP_PARALLEL_PROGRESS 'P' +#define PQMSG_RESP_FUNCTION_CALL'V' +#define PQMSG_RESP_PARAMETER_STATUS 'S' +#define PQMSG_RESP_ROW_DESCRIPTION 'T' +#define PQMSG_RESP_COPY_BOTH'W' +#define PQMSG_RESP_READY_FOR_QUERY 'Z' +#define PQMSG_RESP_NO_DATA 'n' +#define PQMSG_RESP_PASSWORD 'p' +#define PQMSG_RESP_PORTAL_SUSPENDED 's' +#define PQMSG_RESP_PARAMETER_DESCRIPTION 't' +#define PQMSG_RESP_NEGOTIATE_PROTOCOL'v' +#endif Was ordering-by-value intended here? If yes, then FYI both of those groups of #defines are very nearly, but not quite, in that order. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Using defines for protocol characters
On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote: > Dave Cramer writes: >> On Mon, 7 Aug 2023 at 12:59, Robert Haas wrote: >>> PqMsgEmptyQueryResponse or something like that seems better, if we >>> want to keep the current capitalization. I'm not a huge fan of the way >>> we vary our capitalization conventions so much all over the code base, >>> but I think we would at least do well to keep it consistent from one >>> end of a certain identifier to the other. > >> I don't have a strong preference, but before I make the changes I'd like to >> get consensus. >> Can we vote or whatever it takes to decide on a naming pattern that is >> acceptable ? > > I'm good with Robert's proposal above. +1 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Using defines for protocol characters
Dave Cramer writes: > On Mon, 7 Aug 2023 at 12:59, Robert Haas wrote: >> PqMsgEmptyQueryResponse or something like that seems better, if we >> want to keep the current capitalization. I'm not a huge fan of the way >> we vary our capitalization conventions so much all over the code base, >> but I think we would at least do well to keep it consistent from one >> end of a certain identifier to the other. > I don't have a strong preference, but before I make the changes I'd like to > get consensus. > Can we vote or whatever it takes to decide on a naming pattern that is > acceptable ? I'm good with Robert's proposal above. regards, tom lane
Re: Using defines for protocol characters
On Mon, 7 Aug 2023 at 12:59, Robert Haas wrote: > On Mon, Aug 7, 2023 at 2:25 PM Tom Lane wrote: > > +1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse > > and so on? Then one grep would find both uses of the constants and > > code/docs references. Not sure if the prefix should be all-caps or > > not if we go this way. > > PqMsgEmptyQueryResponse or something like that seems better, if we > want to keep the current capitalization. I'm not a huge fan of the way > we vary our capitalization conventions so much all over the code base, > but I think we would at least do well to keep it consistent from one > end of a certain identifier to the other. > I don't have a strong preference, but before I make the changes I'd like to get consensus. Can we vote or whatever it takes to decide on a naming pattern that is acceptable ? Dave
Re: Using defines for protocol characters
On Mon, Aug 7, 2023 at 2:25 PM Tom Lane wrote: > +1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse > and so on? Then one grep would find both uses of the constants and > code/docs references. Not sure if the prefix should be all-caps or > not if we go this way. PqMsgEmptyQueryResponse or something like that seems better, if we want to keep the current capitalization. I'm not a huge fan of the way we vary our capitalization conventions so much all over the code base, but I think we would at least do well to keep it consistent from one end of a certain identifier to the other. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Using defines for protocol characters
On Mon, Aug 07, 2023 at 02:24:58PM -0400, Tom Lane wrote: > Robert Haas writes: >> IMHO, the correspondence between the names in the patch and the >> traditional names in the documentation could be stronger. For example, >> the documentation mentions EmptyQueryResponse and >> NegotiateProtocolVersion, but in this patch those become >> PQMSG_RESP_NEGOTIATE_PROTOCOL and PQMSG_RESP_EMPTY_QUERY. I think we >> could consider directly using the names from the documentation, right >> down to capitalization, perhaps with a prefix, but I'm also totally >> fine with this use of uppercase letters and underscores. But why not >> do a strict translation, like EmptyQueryResponse -> >> PQMSG_EMPTY_QUERY_RESPONSE, NegotiateProtocolVersion -> >> PQMSG_NEGOTIATE_PROTOCOL_VERSION? To me at least, the current patch is >> inventing new and slightly different names for things that already >> have names... > > +1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse > and so on? Then one grep would find both uses of the constants and > code/docs references. Not sure if the prefix should be all-caps or > not if we go this way. +1 for Tom's suggestion. I have no strong opinion about the prefix, but I do think easing greppability is worthwhile. As mentioned earlier [0], I think we should also consider putting the definitions in pqcomm.h. [0] https://postgr.es/m/20230803185356.GA1144430%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Using defines for protocol characters
Robert Haas writes: > IMHO, the correspondence between the names in the patch and the > traditional names in the documentation could be stronger. For example, > the documentation mentions EmptyQueryResponse and > NegotiateProtocolVersion, but in this patch those become > PQMSG_RESP_NEGOTIATE_PROTOCOL and PQMSG_RESP_EMPTY_QUERY. I think we > could consider directly using the names from the documentation, right > down to capitalization, perhaps with a prefix, but I'm also totally > fine with this use of uppercase letters and underscores. But why not > do a strict translation, like EmptyQueryResponse -> > PQMSG_EMPTY_QUERY_RESPONSE, NegotiateProtocolVersion -> > PQMSG_NEGOTIATE_PROTOCOL_VERSION? To me at least, the current patch is > inventing new and slightly different names for things that already > have names... +1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse and so on? Then one grep would find both uses of the constants and code/docs references. Not sure if the prefix should be all-caps or not if we go this way. regards, tom lane
Re: Using defines for protocol characters
On Mon, Aug 7, 2023 at 1:19 PM Dave Cramer wrote: > Any other changes required ? IMHO, the correspondence between the names in the patch and the traditional names in the documentation could be stronger. For example, the documentation mentions EmptyQueryResponse and NegotiateProtocolVersion, but in this patch those become PQMSG_RESP_NEGOTIATE_PROTOCOL and PQMSG_RESP_EMPTY_QUERY. I think we could consider directly using the names from the documentation, right down to capitalization, perhaps with a prefix, but I'm also totally fine with this use of uppercase letters and underscores. But why not do a strict translation, like EmptyQueryResponse -> PQMSG_EMPTY_QUERY_RESPONSE, NegotiateProtocolVersion -> PQMSG_NEGOTIATE_PROTOCOL_VERSION? To me at least, the current patch is inventing new and slightly different names for things that already have names... -- Robert Haas EDB: http://www.enterprisedb.com
Re: Using defines for protocol characters
On Mon, 7 Aug 2023 at 03:10, Alvaro Herrera wrote: > On 2023-Aug-07, Peter Smith wrote: > > > I guess, your patch would not be much different; you can still have > > all the nice names and assign the appropriate values to the enum > > values same as now, but using an enum you might also gain > > type-checking in the code and also get warnings for the "switch" > > statements if there are any cases accidentally omitted. > > Hmm, I think omitting a 'default' clause (which is needed when you want > warnings for missing clauses) in a switch that handles protocol traffic > is not great, because the switch would misbehave when the network > counterpart sends a broken message. I'm not sure we want to do that. > It could become a serious security problem if confronted with a > malicious libpq. > > Any other changes required ? Dave > -- > Álvaro Herrera PostgreSQL Developer — > https://www.EnterpriseDB.com/ >
Re: Using defines for protocol characters
On 2023-Aug-07, Peter Smith wrote: > I guess, your patch would not be much different; you can still have > all the nice names and assign the appropriate values to the enum > values same as now, but using an enum you might also gain > type-checking in the code and also get warnings for the "switch" > statements if there are any cases accidentally omitted. Hmm, I think omitting a 'default' clause (which is needed when you want warnings for missing clauses) in a switch that handles protocol traffic is not great, because the switch would misbehave when the network counterpart sends a broken message. I'm not sure we want to do that. It could become a serious security problem if confronted with a malicious libpq. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Using defines for protocol characters
Hi, I wondered if any consideration was given to using an enum instead of all the #defines. I guess, your patch would not be much different; you can still have all the nice names and assign the appropriate values to the enum values same as now, but using an enum you might also gain type-checking in the code and also get warnings for the "switch" statements if there are any cases accidentally omitted. For example, see typedef enum LogicalRepMsgType [1]. -- [1] https://github.com/postgres/postgres/blob/eeb4eeea2c525c51767ffeafda0070b946f26ae8/src/include/replication/logicalproto.h#L57C31-L57C31 Kind Regards, Peter Smith Fujitsu Australia
Re: Using defines for protocol characters
On Fri, 4 Aug 2023 at 03:44, Alvaro Herrera wrote: > On 2023-Aug-03, Dave Cramer wrote: > > > New patch attached which uses PREPARED_SUB_COMMAND and > > PORTAL_SUB_COMMAND instead > > Hmm, I would keep the prefix in this case and make the message type a > second prefix, with the subtype last -- PQMSG_CLOSE_PREPARED, > PQMSG_DESCRIBE_PORTAL and so on. > I used #define PQMSG_REQ_PREPARED 'S' #define PQMSG_REQ_PORTAL 'P' Duplicating them for CLOSE PREPARED|PORTAL seems awkward > > You define PASSWORD and GSS both with 'p', which I think is bogus; in > some place that isn't doing GSS, you've replaced 'p' with the GSS one > (CheckSASLAuth). I think it'd be better to give 'p' a single name, and > not try to distinguish which is PASSWORD and which is GSS, because > ultimately it's not important. > Done > > There are some unpatched places, such as basebackup_copy.c -- there are > several matches for /'.'/ that correspond to protocol chars in that file. > Also CopySendEndOfRow has one 'd', and desc.c has two 'C'. > > I think fe-trace.c will require further adjustment of the comments for > each function. We could change them to be the symbol for each char, like > so: > > /* PQMSG_RESP_READY_FOR_QUERY */ > static void > pqTraceOutputZ(FILE *f, const char *message, int *cursor) > > Thanks for reviewing see attached for fixes Dave 0001-Created-protocol.h.patch Description: Binary data
Re: Using defines for protocol characters
On 2023-Aug-03, Dave Cramer wrote: > New patch attached which uses PREPARED_SUB_COMMAND and > PORTAL_SUB_COMMAND instead Hmm, I would keep the prefix in this case and make the message type a second prefix, with the subtype last -- PQMSG_CLOSE_PREPARED, PQMSG_DESCRIBE_PORTAL and so on. You define PASSWORD and GSS both with 'p', which I think is bogus; in some place that isn't doing GSS, you've replaced 'p' with the GSS one (CheckSASLAuth). I think it'd be better to give 'p' a single name, and not try to distinguish which is PASSWORD and which is GSS, because ultimately it's not important. There are some unpatched places, such as basebackup_copy.c -- there are several matches for /'.'/ that correspond to protocol chars in that file. Also CopySendEndOfRow has one 'd', and desc.c has two 'C'. I think fe-trace.c will require further adjustment of the comments for each function. We could change them to be the symbol for each char, like so: /* PQMSG_RESP_READY_FOR_QUERY */ static void pqTraceOutputZ(FILE *f, const char *message, int *cursor) Thanks -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Someone said that it is at least an order of magnitude more work to do production software than a prototype. I think he is wrong by at least an order of magnitude." (Brian Kernighan)
Re: Using defines for protocol characters
> On Thu, 3 Aug 2023 at 16:59, Tatsuo Ishii wrote: > >> > On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii wrote: >> > >> >> > Greetings, >> >> > >> >> > Attached is a patch which introduces a file protocol.h. Instead of >> using >> >> > the actual characters everywhere in the code this patch names the >> >> > characters and removes the comments beside each usage. >> >> >> >> > +#define DESCRIBE_PREPARED 'S' >> >> > +#define DESCRIBE_PORTAL 'P' >> >> >> >> You use these for Close message as well. I don't like the idea because >> >> Close is different message from Describe message. >> >> >> >> What about adding following for Close too use them instead? >> >> >> >> #define CLOSE_PREPARED 'S' >> >> #define CLOSE_PORTAL 'P' >> >> >> > >> > Good catch. >> > I recall when writing this it was a bit hacky. >> > What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND >> instead >> > of duplicating them ? >> >> Nice. Looks good to me. >> > New patch attached which uses PREPARED_SUB_COMMAND and > PORTAL_SUB_COMMAND instead > and uses > PQMSG_REQ_* and PQMSG_RESP_* as per Alvaro's suggestion Looks good to me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
On Thu, 3 Aug 2023 at 16:59, Tatsuo Ishii wrote: > > On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii wrote: > > > >> > Greetings, > >> > > >> > Attached is a patch which introduces a file protocol.h. Instead of > using > >> > the actual characters everywhere in the code this patch names the > >> > characters and removes the comments beside each usage. > >> > >> > +#define DESCRIBE_PREPARED 'S' > >> > +#define DESCRIBE_PORTAL 'P' > >> > >> You use these for Close message as well. I don't like the idea because > >> Close is different message from Describe message. > >> > >> What about adding following for Close too use them instead? > >> > >> #define CLOSE_PREPARED 'S' > >> #define CLOSE_PORTAL 'P' > >> > > > > Good catch. > > I recall when writing this it was a bit hacky. > > What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND > instead > > of duplicating them ? > > Nice. Looks good to me. > New patch attached which uses PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead and uses PQMSG_REQ_* and PQMSG_RESP_* as per Alvaro's suggestion Dave Cramer 0001-Created-protocol.h.patch Description: Binary data
Re: Using defines for protocol characters
> On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii wrote: > >> > Greetings, >> > >> > Attached is a patch which introduces a file protocol.h. Instead of using >> > the actual characters everywhere in the code this patch names the >> > characters and removes the comments beside each usage. >> >> > +#define DESCRIBE_PREPARED 'S' >> > +#define DESCRIBE_PORTAL 'P' >> >> You use these for Close message as well. I don't like the idea because >> Close is different message from Describe message. >> >> What about adding following for Close too use them instead? >> >> #define CLOSE_PREPARED 'S' >> #define CLOSE_PORTAL 'P' >> > > Good catch. > I recall when writing this it was a bit hacky. > What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead > of duplicating them ? Nice. Looks good to me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
On Thu, 3 Aug 2023 at 15:22, Dave Cramer wrote: > > > On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii wrote: > >> > Greetings, >> > >> > Attached is a patch which introduces a file protocol.h. Instead of using >> > the actual characters everywhere in the code this patch names the >> > characters and removes the comments beside each usage. >> >> > +#define DESCRIBE_PREPARED 'S' >> > +#define DESCRIBE_PORTAL 'P' >> >> You use these for Close message as well. I don't like the idea because >> Close is different message from Describe message. >> >> What about adding following for Close too use them instead? >> >> #define CLOSE_PREPARED 'S' >> #define CLOSE_PORTAL 'P' >> > > Good catch. > I recall when writing this it was a bit hacky. > What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead > of duplicating them ? > While reviewing this I found a number of mistakes where I used DATA_ROW_RESPONSE instead of DESCRIBE_REQUEST. I can provide a patch now or wait until we resolve the above > > Dave >
Re: Using defines for protocol characters
On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii wrote: > > Greetings, > > > > Attached is a patch which introduces a file protocol.h. Instead of using > > the actual characters everywhere in the code this patch names the > > characters and removes the comments beside each usage. > > > +#define DESCRIBE_PREPARED 'S' > > +#define DESCRIBE_PORTAL 'P' > > You use these for Close message as well. I don't like the idea because > Close is different message from Describe message. > > What about adding following for Close too use them instead? > > #define CLOSE_PREPARED 'S' > #define CLOSE_PORTAL 'P' > Good catch. I recall when writing this it was a bit hacky. What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead of duplicating them ? Dave
Re: Using defines for protocol characters
> Greetings, > > Attached is a patch which introduces a file protocol.h. Instead of using > the actual characters everywhere in the code this patch names the > characters and removes the comments beside each usage. > +#define DESCRIBE_PREPARED 'S' > +#define DESCRIBE_PORTAL 'P' You use these for Close message as well. I don't like the idea because Close is different message from Describe message. What about adding following for Close too use them instead? #define CLOSE_PREPARED 'S' #define CLOSE_PORTAL 'P' Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
On Thu, Aug 03, 2023 at 12:07:21PM -0600, Dave Cramer wrote: > On Thu, 3 Aug 2023 at 11:59, Alvaro Herrera wrote: >> I don't really like the name pattern you've chosen though; I think we >> need to have a common prefix in the defines. Maybe prepending PQMSG_ to >> each name would be enough. And maybe turn the _RESPONSE and _REQUEST >> suffixes you added into prefixes as well, so instead of PARSE_REQUEST >> you could make it PQMSG_REQ_PARSE, PQMSG_RESP_BIND_COMPLETE and so >> on. >> > That becomes trivial to do now that the names are defined. I presumed > someone would object to the names. > I'm fine with the names you propose, but I suggest we wait to see if anyone > objects. I'm okay with the proposed names as well. > + * src/include/protocol.h Could we put these definitions in an existing header such as src/include/libpq/pqcomm.h? I see that's where the authentication request codes live today. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Using defines for protocol characters
On Thu, 3 Aug 2023 at 11:59, Alvaro Herrera wrote: > On 2023-Aug-03, Dave Cramer wrote: > > > Greetings, > > > > Attached is a patch which introduces a file protocol.h. Instead of using > > the actual characters everywhere in the code this patch names the > > characters and removes the comments beside each usage. > > I saw this one last week. I think it's a very idea (and I fact I had > thought of doing it when I last messed with libpq code). > > I don't really like the name pattern you've chosen though; I think we > need to have a common prefix in the defines. Maybe prepending PQMSG_ to > each name would be enough. And maybe turn the _RESPONSE and _REQUEST > suffixes you added into prefixes as well, so instead of PARSE_REQUEST > you could make it PQMSG_REQ_PARSE, PQMSG_RESP_BIND_COMPLETE and so > on. > That becomes trivial to do now that the names are defined. I presumed someone would object to the names. I'm fine with the names you propose, but I suggest we wait to see if anyone objects. Dave > > -- > Álvaro Herrera 48°01'N 7°57'E — > https://www.EnterpriseDB.com/ > It does it in a really, really complicated way > why does it need to be complicated? > Because it's MakeMaker. >
Re: Using defines for protocol characters
On 2023-Aug-03, Dave Cramer wrote: > Greetings, > > Attached is a patch which introduces a file protocol.h. Instead of using > the actual characters everywhere in the code this patch names the > characters and removes the comments beside each usage. I saw this one last week. I think it's a very idea (and I fact I had thought of doing it when I last messed with libpq code). I don't really like the name pattern you've chosen though; I think we need to have a common prefix in the defines. Maybe prepending PQMSG_ to each name would be enough. And maybe turn the _RESPONSE and _REQUEST suffixes you added into prefixes as well, so instead of PARSE_REQUEST you could make it PQMSG_REQ_PARSE, PQMSG_RESP_BIND_COMPLETE and so on. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ It does it in a really, really complicated way why does it need to be complicated? Because it's MakeMaker.
Using defines for protocol characters
Greetings, Attached is a patch which introduces a file protocol.h. Instead of using the actual characters everywhere in the code this patch names the characters and removes the comments beside each usage. Dave Cramer 0001-Created-protocol.h.patch Description: Binary data
