agora inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 1/2] make \d pg_toast.foo show its indices
36+ messages / 11 participants
[nested] [flat]
* [PATCH v2 1/2] make \d pg_toast.foo show its indices
@ 2019-05-03 14:24 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2019-05-03 14:24 UTC (permalink / raw)
---
src/bin/psql/describe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index d7390d5..d26d986 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2274,6 +2274,7 @@ describeOneTableDetails(const char *schemaname,
else if (tableinfo.relkind == RELKIND_RELATION ||
tableinfo.relkind == RELKIND_MATVIEW ||
tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
+ tableinfo.relkind == RELKIND_TOASTVALUE ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
/* Footer information about a table */
--
2.7.4
--brEuL7wsLY8+TuWz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0002-print-table-associated-with-given-TOAST-table.patch"
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v5] make \d pg_toast.foo show its indices
@ 2019-05-03 14:24 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2019-05-03 14:24 UTC (permalink / raw)
---
src/bin/psql/describe.c | 1 +
src/test/regress/expected/psql.out | 2 ++
2 files changed, 3 insertions(+)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 13ed2e1..86a7610 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2303,6 +2303,7 @@ describeOneTableDetails(const char *schemaname,
else if (tableinfo.relkind == RELKIND_RELATION ||
tableinfo.relkind == RELKIND_MATVIEW ||
tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
+ tableinfo.relkind == RELKIND_TOASTVALUE ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
/* Footer information about a table */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 5c8e439..d53dbb0 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -4757,4 +4757,6 @@ TOAST table "pg_toast.pg_toast_2619"
chunk_seq | integer
chunk_data | bytea
For table: "pg_catalog.pg_statistic"
+Indexes:
+ "pg_toast_2619_index" PRIMARY KEY, btree (chunk_id, chunk_seq)
--
2.7.4
--dkEUBIird37B8yKS
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5-0003-show-childs-of-partitioned-indices.patch"
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v5] make \d pg_toast.foo show its indices
@ 2019-05-03 14:24 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2019-05-03 14:24 UTC (permalink / raw)
---
src/bin/psql/describe.c | 1 +
src/test/regress/expected/psql.out | 2 ++
2 files changed, 3 insertions(+)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 13ed2e1..86a7610 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2303,6 +2303,7 @@ describeOneTableDetails(const char *schemaname,
else if (tableinfo.relkind == RELKIND_RELATION ||
tableinfo.relkind == RELKIND_MATVIEW ||
tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
+ tableinfo.relkind == RELKIND_TOASTVALUE ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
/* Footer information about a table */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 5c8e439..d53dbb0 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -4757,4 +4757,6 @@ TOAST table "pg_toast.pg_toast_2619"
chunk_seq | integer
chunk_data | bytea
For table: "pg_catalog.pg_statistic"
+Indexes:
+ "pg_toast_2619_index" PRIMARY KEY, btree (chunk_id, chunk_seq)
--
2.7.4
--Qxx1br4bt0+wmkIi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0003-show-childs-of-partitioned-indices.patch"
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v7] make \d pg_toast.foo show its indices
@ 2019-05-03 14:24 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2019-05-03 14:24 UTC (permalink / raw)
---
src/bin/psql/describe.c | 1 +
src/test/regress/expected/psql.out | 2 ++
2 files changed, 3 insertions(+)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 9cd2e7d..b3b94d1 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2304,6 +2304,7 @@ describeOneTableDetails(const char *schemaname,
else if (tableinfo.relkind == RELKIND_RELATION ||
tableinfo.relkind == RELKIND_MATVIEW ||
tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
+ tableinfo.relkind == RELKIND_TOASTVALUE ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
/* Footer information about a table */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 5c8e439..d53dbb0 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -4757,4 +4757,6 @@ TOAST table "pg_toast.pg_toast_2619"
chunk_seq | integer
chunk_data | bytea
For table: "pg_catalog.pg_statistic"
+Indexes:
+ "pg_toast_2619_index" PRIMARY KEY, btree (chunk_id, chunk_seq)
--
2.7.4
--ryJZkp9/svQ58syV
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0003-show-childs-of-partitioned-indices.patch"
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v3] make \d pg_toast.foo show its indices
@ 2019-05-03 14:24 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2019-05-03 14:24 UTC (permalink / raw)
---
src/bin/psql/describe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index af2f440..c65bc82 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2275,6 +2275,7 @@ describeOneTableDetails(const char *schemaname,
else if (tableinfo.relkind == RELKIND_RELATION ||
tableinfo.relkind == RELKIND_MATVIEW ||
tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
+ tableinfo.relkind == RELKIND_TOASTVALUE ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
/* Footer information about a table */
--
2.7.4
--LpQ9ahxlCli8rRTG
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v3-0002-show-childs-of-partitioned-indices.patch"
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v3] make \d pg_toast.foo show its indices
@ 2019-05-03 14:24 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2019-05-03 14:24 UTC (permalink / raw)
---
src/bin/psql/describe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index af2f440..c65bc82 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2275,6 +2275,7 @@ describeOneTableDetails(const char *schemaname,
else if (tableinfo.relkind == RELKIND_RELATION ||
tableinfo.relkind == RELKIND_MATVIEW ||
tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
+ tableinfo.relkind == RELKIND_TOASTVALUE ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
/* Footer information about a table */
--
2.7.4
--AqsLC8rIMeq19msA
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0002-show-childs-and-tablespaces-of-partitioned-indice.patch"
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, \d and \dP+
It doesn't make much sense that one cannot show a database's default tablespace
without also showing its size, and stat()ing every segment of every relation
in the DB.
---
src/bin/psql/describe.c | 42 ++++++++++++++---------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 53 insertions(+), 43 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee7990ab7e4..44ae6ca9a75 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -224,11 +224,6 @@ describeTablespaces(const char *pattern, int verbose)
appendPQExpBufferStr(&buf, ",\n ");
printACLColumn(&buf, "spcacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
- gettext_noop("Size"));
-
appendPQExpBuffer(&buf,
",\n spcoptions AS \"%s\""
",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
@@ -236,6 +231,11 @@ describeTablespaces(const char *pattern, int verbose)
gettext_noop("Description"));
}
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_tablespace\n");
@@ -907,17 +907,22 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Ctype"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
+
+ if (verbose > 0)
appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\""
",\n t.spcname as \"%s\""
",\n pg_catalog.shobj_description(d.oid, 'pg_database') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Tablespace"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -3780,10 +3785,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -3962,6 +3970,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -3978,9 +3991,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 6428ebc507d..47166727318 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2857,47 +2857,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.17.1
--XLWMkxR+mZNQ4WTO--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, and (for consistency) \d and \dP+
It doesn't make much sense that one cannot show a database's default
tablespace without also showing its size, and stat()ing every segment of
every relation in the DB.
---
src/bin/psql/describe.c | 40 ++++++++++++----------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 49 insertions(+), 45 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 7eff1249015..cc5f00fffe7 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -241,17 +241,15 @@ describeTablespaces(const char *pattern, int verbose)
printACLColumn(&buf, "spcacl");
appendPQExpBuffer(&buf,
- ",\n spcoptions AS \"%s\"",
- gettext_noop("Options"));
+ ",\n spcoptions AS \"%s\""
+ ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
+ gettext_noop("Options"),
+ gettext_noop("Description"));
if (verbose > 1)
appendPQExpBuffer(&buf,
",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
gettext_noop("Size"));
-
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
@@ -972,13 +970,6 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("ICU Rules"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\"",
- gettext_noop("Size"));
if (verbose > 0)
appendPQExpBuffer(&buf,
@@ -987,6 +978,14 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Tablespace"),
gettext_noop("Description"));
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -4012,10 +4011,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -4199,6 +4201,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -4215,9 +4222,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 7cd0c27cca8..e623a2df7c9 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2900,47 +2900,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.34.1
--X119r+cMzLJwahiZ--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, and (for consistency) \d and \dP+
It doesn't make much sense that one cannot show a database's default
tablespace without also showing its size, and stat()ing every segment of
every relation in the DB.
---
src/bin/psql/describe.c | 40 ++++++++++++----------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 49 insertions(+), 45 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 7eff1249015..cc5f00fffe7 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -241,17 +241,15 @@ describeTablespaces(const char *pattern, int verbose)
printACLColumn(&buf, "spcacl");
appendPQExpBuffer(&buf,
- ",\n spcoptions AS \"%s\"",
- gettext_noop("Options"));
+ ",\n spcoptions AS \"%s\""
+ ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
+ gettext_noop("Options"),
+ gettext_noop("Description"));
if (verbose > 1)
appendPQExpBuffer(&buf,
",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
gettext_noop("Size"));
-
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
@@ -972,13 +970,6 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("ICU Rules"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\"",
- gettext_noop("Size"));
if (verbose > 0)
appendPQExpBuffer(&buf,
@@ -987,6 +978,14 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Tablespace"),
gettext_noop("Description"));
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -4012,10 +4011,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -4199,6 +4201,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -4215,9 +4222,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 7cd0c27cca8..e623a2df7c9 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2900,47 +2900,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.34.1
--X119r+cMzLJwahiZ--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, and (for consistency) \d and \dP+
It doesn't make much sense that one cannot show a database's default
tablespace without also showing its size, and stat()ing every segment of
every relation in the DB.
---
src/bin/psql/describe.c | 40 ++++++++++++----------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 49 insertions(+), 45 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 8ab4b4cb46a..1b7e5c5bcd2 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -241,17 +241,15 @@ describeTablespaces(const char *pattern, int verbose)
printACLColumn(&buf, "spcacl");
appendPQExpBuffer(&buf,
- ",\n spcoptions AS \"%s\"",
- gettext_noop("Options"));
+ ",\n spcoptions AS \"%s\""
+ ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
+ gettext_noop("Options"),
+ gettext_noop("Description"));
if (verbose > 1)
appendPQExpBuffer(&buf,
",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
gettext_noop("Size"));
-
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
@@ -972,13 +970,6 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("ICU Rules"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\"",
- gettext_noop("Size"));
if (verbose > 0)
appendPQExpBuffer(&buf,
@@ -987,6 +978,14 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Tablespace"),
gettext_noop("Description"));
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -3951,10 +3950,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -4138,6 +4140,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -4154,9 +4161,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8f930283636..4666333f73f 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2889,47 +2889,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.34.1
--mDjj41GS8UJ6l/Dp--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, \d and \dP+
It doesn't make much sense that one cannot show a database's default tablespace
without also showing its size, and stat()ing every segment of every relation
in the DB.
---
src/bin/psql/describe.c | 42 ++++++++++++++---------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 53 insertions(+), 43 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f7b646062bd..77a883815fe 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -240,11 +240,6 @@ describeTablespaces(const char *pattern, int verbose)
appendPQExpBufferStr(&buf, ",\n ");
printACLColumn(&buf, "spcacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
- gettext_noop("Size"));
-
appendPQExpBuffer(&buf,
",\n spcoptions AS \"%s\""
",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
@@ -252,6 +247,11 @@ describeTablespaces(const char *pattern, int verbose)
gettext_noop("Description"));
}
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_tablespace\n");
@@ -961,17 +961,22 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Locale Provider"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
+
+ if (verbose > 0)
appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\""
",\n t.spcname as \"%s\""
",\n pg_catalog.shobj_description(d.oid, 'pg_database') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Tablespace"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -3941,10 +3946,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -4128,6 +4136,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -4144,9 +4157,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8fc62cebd2d..cd95680994b 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2889,47 +2889,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.25.1
--H1spWtNR+x+ondvy--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, and (for consistency) \d and \dP+
It doesn't make much sense that one cannot show a database's default
tablespace without also showing its size, and stat()ing every segment of
every relation in the DB.
---
src/bin/psql/describe.c | 40 ++++++++++++----------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 49 insertions(+), 45 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 5fc4f5774e8..a64e4ccba4d 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -241,17 +241,15 @@ describeTablespaces(const char *pattern, int verbose)
printACLColumn(&buf, "spcacl");
appendPQExpBuffer(&buf,
- ",\n spcoptions AS \"%s\"",
- gettext_noop("Options"));
+ ",\n spcoptions AS \"%s\""
+ ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
+ gettext_noop("Options"),
+ gettext_noop("Description"));
if (verbose > 1)
appendPQExpBuffer(&buf,
",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
gettext_noop("Size"));
-
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
@@ -972,13 +970,6 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("ICU Rules"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\"",
- gettext_noop("Size"));
if (verbose > 0)
appendPQExpBuffer(&buf,
@@ -987,6 +978,14 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Tablespace"),
gettext_noop("Description"));
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -3954,10 +3953,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -4141,6 +4143,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -4157,9 +4164,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index c00e28361c7..b9c95502a45 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2889,47 +2889,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.34.1
--OZ1/qgOAlc2dTU8W--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v8 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, \d and \dP+
It doesn't make much sense that one cannot show a database's default tablespace
without also showing its size, and stat()ing every segment of every relation
in the DB.
---
src/bin/psql/describe.c | 42 ++++++++++++++---------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 53 insertions(+), 43 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2e47ea448f7..22fc19a47dd 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -240,11 +240,6 @@ describeTablespaces(const char *pattern, int verbose)
appendPQExpBufferStr(&buf, ",\n ");
printACLColumn(&buf, "spcacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
- gettext_noop("Size"));
-
appendPQExpBuffer(&buf,
",\n spcoptions AS \"%s\""
",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
@@ -252,6 +247,11 @@ describeTablespaces(const char *pattern, int verbose)
gettext_noop("Description"));
}
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_tablespace\n");
@@ -961,17 +961,22 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Locale Provider"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
+
+ if (verbose > 0)
appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\""
",\n t.spcname as \"%s\""
",\n pg_catalog.shobj_description(d.oid, 'pg_database') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Tablespace"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -3936,10 +3941,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -4123,6 +4131,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -4139,9 +4152,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index a7f5700edc1..2daa4685b4b 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2858,47 +2858,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.17.1
--cKDw3XFoqocuprIa--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, \d and \dP+
It doesn't make much sense that one cannot show a database's default tablespace
without also showing its size, and stat()ing every segment of every relation
in the DB.
---
src/bin/psql/describe.c | 42 ++++++++++++++---------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 53 insertions(+), 43 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 220262aa84c..9f73e90df23 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -234,11 +234,6 @@ describeTablespaces(const char *pattern, int verbose)
appendPQExpBufferStr(&buf, ",\n ");
printACLColumn(&buf, "spcacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
- gettext_noop("Size"));
-
appendPQExpBuffer(&buf,
",\n spcoptions AS \"%s\""
",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
@@ -246,6 +241,11 @@ describeTablespaces(const char *pattern, int verbose)
gettext_noop("Description"));
}
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_tablespace\n");
@@ -941,17 +941,22 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Locale Provider"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
+
+ if (verbose > 0)
appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\""
",\n t.spcname as \"%s\""
",\n pg_catalog.shobj_description(d.oid, 'pg_database') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Tablespace"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -3898,10 +3903,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -4082,6 +4090,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -4098,9 +4111,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 60acbd1241e..8da23d7216c 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2857,47 +2857,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.17.1
--HcAYCG3uE/tztfnV--
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v4 4/4] Move the double-plus "Size" columns to the right
@ 2021-12-17 15:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:35 UTC (permalink / raw)
\dn, \dA, \db, \l, \d and \dP+
It doesn't make much sense that one cannot show a database's default tablespace
without also showing its size, and stat()ing every segment of every relation
in the DB.
---
src/bin/psql/describe.c | 42 ++++++++++++++---------
src/test/regress/expected/psql.out | 54 +++++++++++++++---------------
2 files changed, 53 insertions(+), 43 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 710bf8ef91..42bad9281e 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -224,11 +224,6 @@ describeTablespaces(const char *pattern, int verbose)
appendPQExpBufferStr(&buf, ",\n ");
printACLColumn(&buf, "spcacl");
- if (verbose > 1)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
- gettext_noop("Size"));
-
appendPQExpBuffer(&buf,
",\n spcoptions AS \"%s\""
",\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \"%s\"",
@@ -236,6 +231,11 @@ describeTablespaces(const char *pattern, int verbose)
gettext_noop("Description"));
}
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_tablespace_size(oid)) AS \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_tablespace\n");
@@ -907,17 +907,22 @@ listAllDbs(const char *pattern, int verbose)
gettext_noop("Ctype"));
appendPQExpBufferStr(&buf, " ");
printACLColumn(&buf, "d.datacl");
- if (verbose > 1)
+
+ if (verbose > 0)
appendPQExpBuffer(&buf,
- ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
- " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
- " ELSE 'No Access'\n"
- " END as \"%s\""
",\n t.spcname as \"%s\""
",\n pg_catalog.shobj_description(d.oid, 'pg_database') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Tablespace"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')\n"
+ " THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))\n"
+ " ELSE 'No Access'\n"
+ " END as \"%s\"",
+ gettext_noop("Size"));
+
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_database d\n");
if (verbose > 0)
@@ -3772,10 +3777,13 @@ listTables(const char *tabtypes, const char *pattern, int verbose, bool showSyst
gettext_noop("Access method"));
appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\""
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Size"),
gettext_noop("Description"));
+
+ if (verbose > 1)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
}
appendPQExpBufferStr(&buf,
@@ -3954,6 +3962,11 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
gettext_noop("Table"));
if (verbose > 0)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
+
+ if (verbose > 1)
{
if (showNested)
{
@@ -3970,9 +3983,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, int verbose)
",\n s.tps as \"%s\"",
gettext_noop("Total size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
}
appendPQExpBufferStr(&buf,
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 930ce8597a..9a521a80b3 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2845,47 +2845,47 @@ Access method: heap
-- AM is displayed for tables, indexes and materialized views.
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent | |
(4 rows)
\dt+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+---------------+-------+----------------------+-------------+---------------+---------+-------------
- tableam_display | tbl_heap | table | regress_display_role | permanent | heap | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+---------------+-------+----------------------+-------------+---------------+-------------
+ tableam_display | tbl_heap | table | regress_display_role | permanent | heap |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent | heap_psql |
(2 rows)
\dm+
- List of relations
- Schema | Name | Type | Owner | Persistence | Access method | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Access method | Description
+-----------------+--------------------+-------------------+----------------------+-------------+---------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | heap_psql |
(1 row)
-- But not for views and sequences.
\dv+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+----------------+------+----------------------+-------------+---------+-------------
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+----------------+------+----------------------+-------------+-------------
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(1 row)
\set HIDE_TABLEAM on
\d+
- List of relations
- Schema | Name | Type | Owner | Persistence | Size | Description
------------------+--------------------+-------------------+----------------------+-------------+---------+-------------
- tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap | table | regress_display_role | permanent | 0 bytes |
- tableam_display | tbl_heap_psql | table | regress_display_role | permanent | 0 bytes |
- tableam_display | view_heap_psql | view | regress_display_role | permanent | 0 bytes |
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Description
+-----------------+--------------------+-------------------+----------------------+-------------+-------------
+ tableam_display | mat_view_heap_psql | materialized view | regress_display_role | permanent |
+ tableam_display | tbl_heap | table | regress_display_role | permanent |
+ tableam_display | tbl_heap_psql | table | regress_display_role | permanent |
+ tableam_display | view_heap_psql | view | regress_display_role | permanent |
(4 rows)
RESET ROLE;
--
2.17.0
--2fEWJT3hVM9yyfvd--
^ permalink raw reply [nested|flat] 36+ messages in thread
* Allow specifying a dbname in pg_basebackup connection string
@ 2023-07-03 12:22 Jelte Fennema <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jelte Fennema @ 2023-07-03 12:22 UTC (permalink / raw)
To: pgsql-hackers
Normally it doesn't really matter which dbname is used in the connection
string that pg_basebackup and other physical replication CLI tools use.
The reason being, that physical replication does not work at the
database level, but instead at the server level. So you will always get
the data for all databases.
However, when there's a proxy, such as PgBouncer, in between the client
and the server, then it might very well matter. Because this proxy might
want to route the connection to a different server depending on the
dbname parameter in the startup packet.
This patch changes the creation of the connection string key value
pairs, so that the following command will actually include
dbname=postgres in the startup packet to the server:
pg_basebackup --dbname 'dbname=postgres port=6432' -D dump
This also applies to other physical replication CLI tools like
pg_receivewal.
To address the security issue described in CVE-2016-5424 it
now only passes expand_dbname=true when the tool did not
receive a connection_string argument.
I tested that the change worked on this PgBouncer PR of mine:
https://github.com/pgbouncer/pgbouncer/pull/876
Attachments:
[application/octet-stream] v1-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch (3.6K, ../../CAGECzQTw-dZkVT_RELRzfWRzY714-VaTjoBATYfZq93R8C-auA@mail.gmail.com/3-v1-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch)
download | inline diff:
From b0fa4c256f637f747f6753738c54f93b89e1b99e Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Mon, 3 Jul 2023 13:50:31 +0200
Subject: [PATCH v1] Allow specifying a dbname in pg_basebackup connection
string
Normally it doesn't really matter which dbname is used in the connection
string that pg_basebackup and other physical replication CLI tools use.
The reason being, that physical replication does not work at the
database level, but instead at the server level. So you will always get
the data for all databases.
However, when there's a proxy, such as PgBouncer, in between the client
and the server, then might very well matter. Because this proxy might
want to route the connection to a different server depending on the
dbname parameter in the startup packet.
This patch changes the creation of the connection string key value
pairs, so that the following command will actually include
dbname=postgres in the startup packet to the server:
```
pg_basebackup --dbname 'dbname=postgres port=6432'
```
This also applies to other physical replication CLI tools like
pg_receivewal.
---
src/bin/pg_basebackup/streamutil.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 15514599c4e..38cae8ff190 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -79,9 +79,6 @@ GetConnection(void)
/*
* Merge the connection info inputs given in form of connection string,
* options and default values (dbname=replication, replication=true, etc.)
- * Explicitly discard any dbname value in the connection string;
- * otherwise, PQconnectdbParams() would interpret that value as being
- * itself a connection string.
*/
i = 0;
if (connection_string)
@@ -92,18 +89,24 @@ GetConnection(void)
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
argcount++;
}
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ /*
+ * Set dbname here already, so it can be overridden by a dbname in the
+ * connection string.
+ */
+ keywords[i] = "dbname";
+ values[i] = "replication";
+ i++;
+
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
{
keywords[i] = conn_opt->keyword;
values[i] = conn_opt->val;
@@ -115,11 +118,11 @@ GetConnection(void)
{
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ keywords[i] = "dbname";
+ values[i] = dbname;
+ i++;
}
- keywords[i] = "dbname";
- values[i] = dbname == NULL ? "replication" : dbname;
- i++;
keywords[i] = "replication";
values[i] = dbname == NULL ? "true" : "database";
i++;
@@ -171,7 +174,11 @@ GetConnection(void)
values[i] = NULL;
}
- tmpconn = PQconnectdbParams(keywords, values, true);
+ /*
+ * Only expand dbname when we did not already parse the argument as a
+ * connection string ourselves.
+ */
+ tmpconn = PQconnectdbParams(keywords, values, !connection_string);
/*
* If there is too little memory even to allocate the PGconn object
base-commit: a4cfeeca5a97f2b5969c31aa69ba775af95ee5a3
--
2.34.1
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-07-05 12:43 Thom Brown <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Thom Brown @ 2023-07-05 12:43 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: pgsql-hackers
On Mon, 3 Jul 2023 at 13:23, Jelte Fennema <[email protected]> wrote:
>
> Normally it doesn't really matter which dbname is used in the connection
> string that pg_basebackup and other physical replication CLI tools use.
> The reason being, that physical replication does not work at the
> database level, but instead at the server level. So you will always get
> the data for all databases.
>
> However, when there's a proxy, such as PgBouncer, in between the client
> and the server, then it might very well matter. Because this proxy might
> want to route the connection to a different server depending on the
> dbname parameter in the startup packet.
>
> This patch changes the creation of the connection string key value
> pairs, so that the following command will actually include
> dbname=postgres in the startup packet to the server:
>
> pg_basebackup --dbname 'dbname=postgres port=6432' -D dump
I guess my immediate question is, should backups be taken through
PgBouncer? It seems beyond PgBouncer's remit.
Thom
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-07-05 14:01 Euler Taveira <[email protected]>
parent: Thom Brown <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Euler Taveira @ 2023-07-05 14:01 UTC (permalink / raw)
To: Thom Brown <[email protected]>; Jelte Fennema <[email protected]>; +Cc: pgsql-hackers
On Wed, Jul 5, 2023, at 9:43 AM, Thom Brown wrote:
> I guess my immediate question is, should backups be taken through
> PgBouncer? It seems beyond PgBouncer's remit.
One of the PgBouncer's missions is to be a transparent proxy.
Sometimes you cannot reach out the database directly due to a security policy.
I've heard this backup question a few times. IMO if dbname doesn't matter for
reaching the server directly, I don't see a problem relaxing this restriction
to support this use case. We just need to document that dbname will be ignored
if specified. Other connection poolers might also benefit from it.
--
Euler Taveira
EDB https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-07-05 15:50 Jelte Fennema <[email protected]>
parent: Euler Taveira <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jelte Fennema @ 2023-07-05 15:50 UTC (permalink / raw)
To: Euler Taveira <[email protected]>; +Cc: Thom Brown <[email protected]>; pgsql-hackers
On Wed, 5 Jul 2023 at 16:01, Euler Taveira <[email protected]> wrote:
> One of the PgBouncer's missions is to be a transparent proxy.
>
> Sometimes you cannot reach out the database directly due to a security policy.
Indeed the transparent proxy use case is where replication through
pgbouncer makes sense. There's quite some reasons to set up PgBouncer
like such a proxy apart from security policies. Some others that come
to mind are:
- load balancer layer of pgbouncers
- transparent failovers
- transparent database moves
And in all of those cases its nice for a user to use a single
connection string/hostname. Instead of having to think: Oh yeah, for
backups, I need to use this other one.
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-07-05 18:08 Thom Brown <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Thom Brown @ 2023-07-05 18:08 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: Euler Taveira <[email protected]>; pgsql-hackers
On Wed, 5 Jul 2023 at 16:50, Jelte Fennema <[email protected]> wrote:
>
> On Wed, 5 Jul 2023 at 16:01, Euler Taveira <[email protected]> wrote:
> > One of the PgBouncer's missions is to be a transparent proxy.
> >
> > Sometimes you cannot reach out the database directly due to a security policy.
>
> Indeed the transparent proxy use case is where replication through
> pgbouncer makes sense. There's quite some reasons to set up PgBouncer
> like such a proxy apart from security policies. Some others that come
> to mind are:
> - load balancer layer of pgbouncers
> - transparent failovers
> - transparent database moves
>
> And in all of those cases its nice for a user to use a single
> connection string/hostname. Instead of having to think: Oh yeah, for
> backups, I need to use this other one.
Okay, understood. In that case, please remember to write changes to
the pg_basebackup docs page explaining how the dbname value is ignored
under normal usage.
Thom
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-07-05 19:39 Jelte Fennema <[email protected]>
parent: Thom Brown <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jelte Fennema @ 2023-07-05 19:39 UTC (permalink / raw)
To: Thom Brown <[email protected]>; +Cc: Euler Taveira <[email protected]>; pgsql-hackers
On Wed, 5 Jul 2023 at 20:09, Thom Brown <[email protected]> wrote:
> Okay, understood. In that case, please remember to write changes to
> the pg_basebackup docs page explaining how the dbname value is ignored
I updated the wording in the docs for pg_basebackup and pg_receivewal.
They now clarify that Postgres itself doesn't care if there's a
database name in the connection string, but that a proxy might.
Attachments:
[application/octet-stream] v2-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch (5.9K, ../../CAGECzQTH6Nqsgq4-bRAD2N-QQmtF30bQMQ=Y=NKp2A41Zd8TLA@mail.gmail.com/2-v2-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch)
download | inline diff:
From 753d0862eafb0739acba513be83aff359351f315 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Mon, 3 Jul 2023 13:50:31 +0200
Subject: [PATCH v2] Allow specifying a dbname in pg_basebackup connection
string
Normally it doesn't really matter which dbname is used in the connection
string that pg_basebackup and other physical replication CLI tools use.
The reason being, that physical replication does not work at the
database level, but instead at the server level. So you will always get
the data for all databases.
However, when there's a proxy, such as PgBouncer, in between the client
and the server, then might very well matter. Because this proxy might
want to route the connection to a different server depending on the
dbname parameter in the startup packet.
This patch changes the creation of the connection string key value
pairs, so that the following command will actually include
dbname=postgres in the startup packet to the server:
```
pg_basebackup --dbname 'dbname=postgres port=6432'
```
This also applies to other physical replication CLI tools like
pg_receivewal.
---
doc/src/sgml/ref/pg_basebackup.sgml | 9 +++++++--
doc/src/sgml/ref/pg_receivewal.sgml | 9 +++++++--
src/bin/pg_basebackup/streamutil.c | 29 ++++++++++++++++++-----------
3 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 79d3e657c32..685bfc6e2c0 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -752,8 +752,13 @@ PostgreSQL documentation
<para>
The option is called <literal>--dbname</literal> for consistency with other
client applications, but because <application>pg_basebackup</application>
- doesn't connect to any particular database in the cluster, any database
- name in the connection string will be ignored.
+ doesn't connect to any particular database in the cluster, supplying a
+ specific database name in the connection string won't cause
+ <productname>PostgreSQL</productname> to behave any differently.
+ However, if you are connecting to <productname>PostgreSQL</productname>
+ through a proxy, then it's possible that this proxy does use the
+ supplied databasename to make certain decisions, such as to which
+ cluster to route the connection.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index cecc7daec97..9413543099b 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -316,8 +316,13 @@ PostgreSQL documentation
<para>
The option is called <literal>--dbname</literal> for consistency with other
client applications, but because <application>pg_receivewal</application>
- doesn't connect to any particular database in the cluster, database
- name in the connection string will be ignored.
+ doesn't connect to any particular database in the cluster, supplying a
+ specific database name in the connection string won't cause
+ <productname>PostgreSQL</productname> to behave any differently.
+ However, if you are connecting to <productname>PostgreSQL</productname>
+ through a proxy, then it's possible that this proxy does use the
+ supplied databasename to make certain decisions, such as to which
+ cluster to route the connection.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 15514599c4e..38cae8ff190 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -79,9 +79,6 @@ GetConnection(void)
/*
* Merge the connection info inputs given in form of connection string,
* options and default values (dbname=replication, replication=true, etc.)
- * Explicitly discard any dbname value in the connection string;
- * otherwise, PQconnectdbParams() would interpret that value as being
- * itself a connection string.
*/
i = 0;
if (connection_string)
@@ -92,18 +89,24 @@ GetConnection(void)
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
argcount++;
}
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ /*
+ * Set dbname here already, so it can be overridden by a dbname in the
+ * connection string.
+ */
+ keywords[i] = "dbname";
+ values[i] = "replication";
+ i++;
+
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
{
keywords[i] = conn_opt->keyword;
values[i] = conn_opt->val;
@@ -115,11 +118,11 @@ GetConnection(void)
{
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ keywords[i] = "dbname";
+ values[i] = dbname;
+ i++;
}
- keywords[i] = "dbname";
- values[i] = dbname == NULL ? "replication" : dbname;
- i++;
keywords[i] = "replication";
values[i] = dbname == NULL ? "true" : "database";
i++;
@@ -171,7 +174,11 @@ GetConnection(void)
values[i] = NULL;
}
- tmpconn = PQconnectdbParams(keywords, values, true);
+ /*
+ * Only expand dbname when we did not already parse the argument as a
+ * connection string ourselves.
+ */
+ tmpconn = PQconnectdbParams(keywords, values, !connection_string);
/*
* If there is too little memory even to allocate the PGconn object
base-commit: 4f4d73466d71976b58f29121bab9d9fef6f3436e
--
2.34.1
^ permalink raw reply [nested|flat] 36+ messages in thread
* [PATCH v6 1/1] Introduce macros for protocol characters.
@ 2023-08-16 19:08 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Nathan Bossart @ 2023-08-16 19:08 UTC (permalink / raw)
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');
+ pq_beginmessage(&msgbuf, PqMsg_BackendKeyData);
pq_sendint32(&msgbuf, (int32) MyProcPid);
pq_sendint32(&msgbuf, (int32) MyCancelKey);
pq_endmessage(&msgbuf);
@@ -1550,7 +1550,7 @@ ParallelWorkerMain(Datum main_arg)
DetachSession();
/* Report success. */
- pq_putmessage('X', NULL, 0);
+ pq_putmessage(PqMsg_Terminate, NULL, 0);
}
/*
diff --git a/src/backend/backup/basebackup_copy.c b/src/backend/backup/basebackup_copy.c
index 1db80cde1b..fee30c21e1 100644
--- a/src/backend/backup/basebackup_copy.c
+++ b/src/backend/backup/basebackup_copy.c
@@ -152,7 +152,7 @@ bbsink_copystream_begin_backup(bbsink *sink)
SendTablespaceList(state->tablespaces);
/* Send a CommandComplete message */
- pq_puttextmessage('C', "SELECT");
+ pq_puttextmessage(PqMsg_CommandComplete, "SELECT");
/* Begin COPY stream. This will be used for all archives + manifest. */
SendCopyOutResponse();
@@ -169,7 +169,7 @@ bbsink_copystream_begin_archive(bbsink *sink, const char *archive_name)
StringInfoData buf;
ti = list_nth(state->tablespaces, state->tablespace_num);
- pq_beginmessage(&buf, 'd'); /* CopyData */
+ pq_beginmessage(&buf, PqMsg_CopyData);
pq_sendbyte(&buf, 'n'); /* New archive */
pq_sendstring(&buf, archive_name);
pq_sendstring(&buf, ti->path == NULL ? "" : ti->path);
@@ -220,7 +220,7 @@ bbsink_copystream_archive_contents(bbsink *sink, size_t len)
{
mysink->last_progress_report_time = now;
- pq_beginmessage(&buf, 'd'); /* CopyData */
+ pq_beginmessage(&buf, PqMsg_CopyData);
pq_sendbyte(&buf, 'p'); /* Progress report */
pq_sendint64(&buf, state->bytes_done);
pq_endmessage(&buf);
@@ -246,7 +246,7 @@ bbsink_copystream_end_archive(bbsink *sink)
mysink->bytes_done_at_last_time_check = state->bytes_done;
mysink->last_progress_report_time = GetCurrentTimestamp();
- pq_beginmessage(&buf, 'd'); /* CopyData */
+ pq_beginmessage(&buf, PqMsg_CopyData);
pq_sendbyte(&buf, 'p'); /* Progress report */
pq_sendint64(&buf, state->bytes_done);
pq_endmessage(&buf);
@@ -261,7 +261,7 @@ bbsink_copystream_begin_manifest(bbsink *sink)
{
StringInfoData buf;
- pq_beginmessage(&buf, 'd'); /* CopyData */
+ pq_beginmessage(&buf, PqMsg_CopyData);
pq_sendbyte(&buf, 'm'); /* Manifest */
pq_endmessage(&buf);
}
@@ -318,7 +318,7 @@ SendCopyOutResponse(void)
{
StringInfoData buf;
- pq_beginmessage(&buf, 'H');
+ pq_beginmessage(&buf, PqMsg_CopyOutResponse);
pq_sendbyte(&buf, 0); /* overall format */
pq_sendint16(&buf, 0); /* natts */
pq_endmessage(&buf);
@@ -330,7 +330,7 @@ SendCopyOutResponse(void)
static void
SendCopyDone(void)
{
- pq_putemptymessage('c');
+ pq_putemptymessage(PqMsg_CopyDone);
}
/*
@@ -368,7 +368,7 @@ SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli)
end_tup_output(tstate);
/* Send a CommandComplete message */
- pq_puttextmessage('C', "SELECT");
+ pq_puttextmessage(PqMsg_CommandComplete, "SELECT");
}
/*
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index ef909cf4e0..d148d10850 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -2281,7 +2281,7 @@ NotifyMyFrontEnd(const char *channel, const char *payload, int32 srcPid)
{
StringInfoData buf;
- pq_beginmessage(&buf, 'A');
+ pq_beginmessage(&buf, PqMsg_NotificationResponse);
pq_sendint32(&buf, srcPid);
pq_sendstring(&buf, channel);
pq_sendstring(&buf, payload);
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 232768a6e1..f553734582 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -174,7 +174,7 @@ ReceiveCopyBegin(CopyFromState cstate)
int16 format = (cstate->opts.binary ? 1 : 0);
int i;
- pq_beginmessage(&buf, 'G');
+ pq_beginmessage(&buf, PqMsg_CopyInResponse);
pq_sendbyte(&buf, format); /* overall format */
pq_sendint16(&buf, natts);
for (i = 0; i < natts; i++)
@@ -279,13 +279,13 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
/* Validate message type and set packet size limit */
switch (mtype)
{
- case 'd': /* CopyData */
+ case PqMsg_CopyData:
maxmsglen = PQ_LARGE_MESSAGE_LIMIT;
break;
- case 'c': /* CopyDone */
- case 'f': /* CopyFail */
- case 'H': /* Flush */
- case 'S': /* Sync */
+ case PqMsg_CopyDone:
+ case PqMsg_CopyFail:
+ case PqMsg_Flush:
+ case PqMsg_Sync:
maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
break;
default:
@@ -305,20 +305,20 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
/* ... and process it */
switch (mtype)
{
- case 'd': /* CopyData */
+ case PqMsg_CopyData:
break;
- case 'c': /* CopyDone */
+ case PqMsg_CopyDone:
/* COPY IN correctly terminated by frontend */
cstate->raw_reached_eof = true;
return bytesread;
- case 'f': /* CopyFail */
+ case PqMsg_CopyFail:
ereport(ERROR,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("COPY from stdin failed: %s",
pq_getmsgstring(cstate->fe_msgbuf))));
break;
- case 'H': /* Flush */
- case 'S': /* Sync */
+ case PqMsg_Flush:
+ case PqMsg_Sync:
/*
* Ignore Flush/Sync for the convenience of client
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 9e4b2437a5..eaa3172793 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -144,7 +144,7 @@ SendCopyBegin(CopyToState cstate)
int16 format = (cstate->opts.binary ? 1 : 0);
int i;
- pq_beginmessage(&buf, 'H');
+ pq_beginmessage(&buf, PqMsg_CopyOutResponse);
pq_sendbyte(&buf, format); /* overall format */
pq_sendint16(&buf, natts);
for (i = 0; i < natts; i++)
@@ -159,7 +159,7 @@ SendCopyEnd(CopyToState cstate)
/* Shouldn't have any unsent data */
Assert(cstate->fe_msgbuf->len == 0);
/* Send Copy Done message */
- pq_putemptymessage('c');
+ pq_putemptymessage(PqMsg_CopyDone);
}
/*----------
@@ -247,7 +247,7 @@ CopySendEndOfRow(CopyToState cstate)
CopySendChar(cstate, '\n');
/* Dump the accumulated row as one CopyData message */
- (void) pq_putmessage('d', fe_msgbuf->data, fe_msgbuf->len);
+ (void) pq_putmessage(PqMsg_CopyData, fe_msgbuf->data, fe_msgbuf->len);
break;
case COPY_CALLBACK:
cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len);
diff --git a/src/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c
index 684680897b..c535bc5383 100644
--- a/src/backend/libpq/auth-sasl.c
+++ b/src/backend/libpq/auth-sasl.c
@@ -87,7 +87,7 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass,
{
pq_startmsgread();
mtype = pq_getbyte();
- if (mtype != 'p')
+ if (mtype != PqMsg_SASLResponse)
{
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 315a24bb3f..0356fe3e45 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -665,7 +665,7 @@ sendAuthRequest(Port *port, AuthRequest areq, const char *extradata, int extrale
CHECK_FOR_INTERRUPTS();
- pq_beginmessage(&buf, 'R');
+ pq_beginmessage(&buf, PqMsg_AuthenticationRequest);
pq_sendint32(&buf, (int32) areq);
if (extralen > 0)
pq_sendbytes(&buf, extradata, extralen);
@@ -698,7 +698,7 @@ recv_password_packet(Port *port)
/* Expect 'p' message type */
mtype = pq_getbyte();
- if (mtype != 'p')
+ if (mtype != PqMsg_PasswordMessage)
{
/*
* If the client just disconnects without offering a password, don't
@@ -961,7 +961,7 @@ pg_GSS_recvauth(Port *port)
CHECK_FOR_INTERRUPTS();
mtype = pq_getbyte();
- if (mtype != 'p')
+ if (mtype != PqMsg_GSSResponse)
{
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
@@ -1232,7 +1232,7 @@ pg_SSPI_recvauth(Port *port)
{
pq_startmsgread();
mtype = pq_getbyte();
- if (mtype != 'p')
+ if (mtype != PqMsg_GSSResponse)
{
if (sspictx != NULL)
{
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9c8ec779f9..07d376d77e 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2357,7 +2357,7 @@ SendNegotiateProtocolVersion(List *unrecognized_protocol_options)
StringInfoData buf;
ListCell *lc;
- pq_beginmessage(&buf, 'v'); /* NegotiateProtocolVersion */
+ pq_beginmessage(&buf, PqMsg_NegotiateProtocolVersion);
pq_sendint32(&buf, PG_PROTOCOL_LATEST);
pq_sendint32(&buf, list_length(unrecognized_protocol_options));
foreach(lc, unrecognized_protocol_options)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index d27ef2985d..80374c55be 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -603,7 +603,7 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
dest->rStartup(dest, CMD_SELECT, tupdesc);
/* Send a DataRow message */
- pq_beginmessage(&buf, 'D');
+ pq_beginmessage(&buf, PqMsg_DataRow);
pq_sendint16(&buf, 2); /* # of columns */
len = strlen(histfname);
pq_sendint32(&buf, len); /* col1 len */
@@ -801,7 +801,7 @@ StartReplication(StartReplicationCmd *cmd)
WalSndSetState(WALSNDSTATE_CATCHUP);
/* Send a CopyBothResponse message, and start streaming */
- pq_beginmessage(&buf, 'W');
+ pq_beginmessage(&buf, PqMsg_CopyBothResponse);
pq_sendbyte(&buf, 0);
pq_sendint16(&buf, 0);
pq_endmessage(&buf);
@@ -1294,7 +1294,7 @@ StartLogicalReplication(StartReplicationCmd *cmd)
WalSndSetState(WALSNDSTATE_CATCHUP);
/* Send a CopyBothResponse message, and start streaming */
- pq_beginmessage(&buf, 'W');
+ pq_beginmessage(&buf, PqMsg_CopyBothResponse);
pq_sendbyte(&buf, 0);
pq_sendint16(&buf, 0);
pq_endmessage(&buf);
@@ -1923,11 +1923,11 @@ ProcessRepliesIfAny(void)
/* Validate message type and set packet size limit */
switch (firstchar)
{
- case 'd':
+ case PqMsg_CopyData:
maxmsglen = PQ_LARGE_MESSAGE_LIMIT;
break;
- case 'c':
- case 'X':
+ case PqMsg_CopyDone:
+ case PqMsg_Terminate:
maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
break;
default:
@@ -1955,7 +1955,7 @@ ProcessRepliesIfAny(void)
/*
* 'd' means a standby reply wrapped in a CopyData packet.
*/
- case 'd':
+ case PqMsg_CopyData:
ProcessStandbyMessage();
received = true;
break;
@@ -1964,7 +1964,7 @@ ProcessRepliesIfAny(void)
* CopyDone means the standby requested to finish streaming.
* Reply with CopyDone, if we had not sent that already.
*/
- case 'c':
+ case PqMsg_CopyDone:
if (!streamingDoneSending)
{
pq_putmessage_noblock('c', NULL, 0);
@@ -1978,7 +1978,7 @@ ProcessRepliesIfAny(void)
/*
* 'X' means that the standby is closing down the socket.
*/
- case 'X':
+ case PqMsg_Terminate:
proc_exit(0);
default:
diff --git a/src/backend/tcop/dest.c b/src/backend/tcop/dest.c
index c0406e2ee5..06d1872b9a 100644
--- a/src/backend/tcop/dest.c
+++ b/src/backend/tcop/dest.c
@@ -176,7 +176,7 @@ EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_o
len = BuildQueryCompletionString(completionTag, qc,
force_undecorated_output);
- pq_putmessage('C', completionTag, len + 1);
+ pq_putmessage(PqMsg_Close, completionTag, len + 1);
case DestNone:
case DestDebug:
@@ -200,7 +200,7 @@ EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_o
void
EndReplicationCommand(const char *commandTag)
{
- pq_putmessage('C', commandTag, strlen(commandTag) + 1);
+ pq_putmessage(PqMsg_Close, commandTag, strlen(commandTag) + 1);
}
/* ----------------
@@ -220,7 +220,7 @@ NullCommand(CommandDest dest)
case DestRemoteSimple:
/* Tell the FE that we saw an empty query string */
- pq_putemptymessage('I');
+ pq_putemptymessage(PqMsg_EmptyQueryResponse);
break;
case DestNone:
@@ -258,7 +258,7 @@ ReadyForQuery(CommandDest dest)
{
StringInfoData buf;
- pq_beginmessage(&buf, 'Z');
+ pq_beginmessage(&buf, PqMsg_ReadyForQuery);
pq_sendbyte(&buf, TransactionBlockStatusCode());
pq_endmessage(&buf);
}
diff --git a/src/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c
index 2f70ebd5fa..71f161dbe2 100644
--- a/src/backend/tcop/fastpath.c
+++ b/src/backend/tcop/fastpath.c
@@ -69,7 +69,7 @@ SendFunctionResult(Datum retval, bool isnull, Oid rettype, int16 format)
{
StringInfoData buf;
- pq_beginmessage(&buf, 'V');
+ pq_beginmessage(&buf, PqMsg_FunctionCallResponse);
if (isnull)
{
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 36cc99ec9c..e4756f8be2 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -402,37 +402,37 @@ SocketBackend(StringInfo inBuf)
*/
switch (qtype)
{
- case 'Q': /* simple query */
+ case PqMsg_Query:
maxmsglen = PQ_LARGE_MESSAGE_LIMIT;
doing_extended_query_message = false;
break;
- case 'F': /* fastpath function call */
+ case PqMsg_FunctionCall:
maxmsglen = PQ_LARGE_MESSAGE_LIMIT;
doing_extended_query_message = false;
break;
- case 'X': /* terminate */
+ case PqMsg_Terminate:
maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
doing_extended_query_message = false;
ignore_till_sync = false;
break;
- case 'B': /* bind */
- case 'P': /* parse */
+ case PqMsg_Bind:
+ case PqMsg_Parse:
maxmsglen = PQ_LARGE_MESSAGE_LIMIT;
doing_extended_query_message = true;
break;
- case 'C': /* close */
- case 'D': /* describe */
- case 'E': /* execute */
- case 'H': /* flush */
+ case PqMsg_Close:
+ case PqMsg_Describe:
+ case PqMsg_Execute:
+ case PqMsg_Flush:
maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
doing_extended_query_message = true;
break;
- case 'S': /* sync */
+ case PqMsg_Sync:
maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
/* stop any active skip-till-Sync */
ignore_till_sync = false;
@@ -440,13 +440,13 @@ SocketBackend(StringInfo inBuf)
doing_extended_query_message = false;
break;
- case 'd': /* copy data */
+ case PqMsg_CopyData:
maxmsglen = PQ_LARGE_MESSAGE_LIMIT;
doing_extended_query_message = false;
break;
- case 'c': /* copy done */
- case 'f': /* copy fail */
+ case PqMsg_CopyDone:
+ case PqMsg_CopyFail:
maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
doing_extended_query_message = false;
break;
@@ -1589,7 +1589,7 @@ exec_parse_message(const char *query_string, /* string to execute */
* Send ParseComplete.
*/
if (whereToSendOutput == DestRemote)
- pq_putemptymessage('1');
+ pq_putemptymessage(PqMsg_ParseComplete);
/*
* Emit duration logging if appropriate.
@@ -2047,7 +2047,7 @@ exec_bind_message(StringInfo input_message)
* Send BindComplete.
*/
if (whereToSendOutput == DestRemote)
- pq_putemptymessage('2');
+ pq_putemptymessage(PqMsg_BindComplete);
/*
* Emit duration logging if appropriate.
@@ -2290,7 +2290,7 @@ exec_execute_message(const char *portal_name, long max_rows)
{
/* Portal run not complete, so send PortalSuspended */
if (whereToSendOutput == DestRemote)
- pq_putemptymessage('s');
+ pq_putemptymessage(PqMsg_PortalSuspended);
/*
* Set XACT_FLAGS_PIPELINING whenever we suspend an Execute message,
@@ -2683,7 +2683,7 @@ exec_describe_statement_message(const char *stmt_name)
NULL);
}
else
- pq_putemptymessage('n'); /* NoData */
+ pq_putemptymessage(PqMsg_NoData);
}
/*
@@ -2736,7 +2736,7 @@ exec_describe_portal_message(const char *portal_name)
FetchPortalTargetList(portal),
portal->formats);
else
- pq_putemptymessage('n'); /* NoData */
+ pq_putemptymessage(PqMsg_NoData);
}
@@ -4239,7 +4239,7 @@ PostgresMain(const char *dbname, const char *username)
{
StringInfoData buf;
- pq_beginmessage(&buf, 'K');
+ pq_beginmessage(&buf, PqMsg_BackendKeyData);
pq_sendint32(&buf, (int32) MyProcPid);
pq_sendint32(&buf, (int32) MyCancelKey);
pq_endmessage(&buf);
@@ -4618,7 +4618,7 @@ PostgresMain(const char *dbname, const char *username)
switch (firstchar)
{
- case 'Q': /* simple query */
+ case PqMsg_Query:
{
const char *query_string;
@@ -4642,7 +4642,7 @@ PostgresMain(const char *dbname, const char *username)
}
break;
- case 'P': /* parse */
+ case PqMsg_Parse:
{
const char *stmt_name;
const char *query_string;
@@ -4672,7 +4672,7 @@ PostgresMain(const char *dbname, const char *username)
}
break;
- case 'B': /* bind */
+ case PqMsg_Bind:
forbidden_in_wal_sender(firstchar);
/* Set statement_timestamp() */
@@ -4687,7 +4687,7 @@ PostgresMain(const char *dbname, const char *username)
/* exec_bind_message does valgrind_report_error_query */
break;
- case 'E': /* execute */
+ case PqMsg_Execute:
{
const char *portal_name;
int max_rows;
@@ -4707,7 +4707,7 @@ PostgresMain(const char *dbname, const char *username)
}
break;
- case 'F': /* fastpath function call */
+ case PqMsg_FunctionCall:
forbidden_in_wal_sender(firstchar);
/* Set statement_timestamp() */
@@ -4742,7 +4742,7 @@ PostgresMain(const char *dbname, const char *username)
send_ready_for_query = true;
break;
- case 'C': /* close */
+ case PqMsg_Close:
{
int close_type;
const char *close_target;
@@ -4782,13 +4782,13 @@ PostgresMain(const char *dbname, const char *username)
}
if (whereToSendOutput == DestRemote)
- pq_putemptymessage('3'); /* CloseComplete */
+ pq_putemptymessage(PqMsg_CloseComplete);
valgrind_report_error_query("CLOSE message");
}
break;
- case 'D': /* describe */
+ case PqMsg_Describe:
{
int describe_type;
const char *describe_target;
@@ -4822,13 +4822,13 @@ PostgresMain(const char *dbname, const char *username)
}
break;
- case 'H': /* flush */
+ case PqMsg_Flush:
pq_getmsgend(&input_message);
if (whereToSendOutput == DestRemote)
pq_flush();
break;
- case 'S': /* sync */
+ case PqMsg_Sync:
pq_getmsgend(&input_message);
finish_xact_command();
valgrind_report_error_query("SYNC message");
@@ -4847,7 +4847,7 @@ PostgresMain(const char *dbname, const char *username)
/* FALLTHROUGH */
- case 'X':
+ case PqMsg_Terminate:
/*
* Reset whereToSendOutput to prevent ereport from attempting
@@ -4865,9 +4865,9 @@ PostgresMain(const char *dbname, const char *username)
*/
proc_exit(0);
- case 'd': /* copy data */
- case 'c': /* copy done */
- case 'f': /* copy fail */
+ case PqMsg_CopyData:
+ case PqMsg_CopyDone:
+ case PqMsg_CopyFail:
/*
* Accept but ignore these messages, per protocol spec; we
@@ -4897,7 +4897,7 @@ forbidden_in_wal_sender(char firstchar)
{
if (am_walsender)
{
- if (firstchar == 'F')
+ if (firstchar == PqMsg_FunctionCall)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("fastpath function calls not supported in a replication connection")));
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..8e1f3e8521 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3465,7 +3465,10 @@ send_message_to_frontend(ErrorData *edata)
char tbuf[12];
/* 'N' (Notice) is for nonfatal conditions, 'E' is for errors */
- pq_beginmessage(&msgbuf, (edata->elevel < ERROR) ? 'N' : 'E');
+ if (edata->elevel < ERROR)
+ pq_beginmessage(&msgbuf, PqMsg_NoticeResponse);
+ else
+ pq_beginmessage(&msgbuf, PqMsg_ErrorResponse);
sev = error_severity(edata->elevel);
pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 99bb2fdd19..84e7ad4d90 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2593,7 +2593,7 @@ ReportGUCOption(struct config_generic *record)
{
StringInfoData msgbuf;
- pq_beginmessage(&msgbuf, 'S');
+ pq_beginmessage(&msgbuf, PqMsg_ParameterStatus);
pq_sendstring(&msgbuf, record->name);
pq_sendstring(&msgbuf, val);
pq_endmessage(&msgbuf);
diff --git a/src/include/Makefile b/src/include/Makefile
index 5d213187e2..2d5242561c 100644
--- a/src/include/Makefile
+++ b/src/include/Makefile
@@ -40,6 +40,7 @@ install: all installdirs
$(INSTALL_DATA) $(srcdir)/port.h '$(DESTDIR)$(includedir_internal)'
$(INSTALL_DATA) $(srcdir)/postgres_fe.h '$(DESTDIR)$(includedir_internal)'
$(INSTALL_DATA) $(srcdir)/libpq/pqcomm.h '$(DESTDIR)$(includedir_internal)/libpq'
+ $(INSTALL_DATA) $(srcdir)/libpq/protocol.h '$(DESTDIR)$(includedir_internal)/libpq'
# These headers are needed for server-side development
$(INSTALL_DATA) pg_config.h '$(DESTDIR)$(includedir_server)'
$(INSTALL_DATA) pg_config_ext.h '$(DESTDIR)$(includedir_server)'
@@ -65,7 +66,7 @@ installdirs:
uninstall:
rm -f $(addprefix '$(DESTDIR)$(includedir)'/, pg_config.h pg_config_ext.h pg_config_os.h pg_config_manual.h postgres_ext.h libpq/libpq-fs.h)
- rm -f $(addprefix '$(DESTDIR)$(includedir_internal)'/, c.h port.h postgres_fe.h libpq/pqcomm.h)
+ rm -f $(addprefix '$(DESTDIR)$(includedir_internal)'/, c.h port.h postgres_fe.h libpq/pqcomm.h libpq/protocol.h)
# heuristic...
rm -rf $(addprefix '$(DESTDIR)$(includedir_server)'/, $(SUBDIRS) *.h)
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 3da00f7983..46a0946b8b 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -21,6 +21,12 @@
#include <netdb.h>
#include <netinet/in.h>
+/*
+ * The definitions for the request/response codes are kept in a separate file
+ * for ease of use in third party programs.
+ */
+#include "libpq/protocol.h"
+
typedef struct
{
struct sockaddr_storage addr;
@@ -112,23 +118,6 @@ typedef uint32 PacketLen;
#define MAX_STARTUP_PACKET_LENGTH 10000
-/* These are the authentication request codes sent by the backend. */
-
-#define AUTH_REQ_OK 0 /* User is authenticated */
-#define AUTH_REQ_KRB4 1 /* Kerberos V4. Not supported any more. */
-#define AUTH_REQ_KRB5 2 /* Kerberos V5. Not supported any more. */
-#define AUTH_REQ_PASSWORD 3 /* Password */
-#define AUTH_REQ_CRYPT 4 /* crypt password. Not supported any more. */
-#define AUTH_REQ_MD5 5 /* md5 password */
-/* 6 is available. It was used for SCM creds, not supported any more. */
-#define AUTH_REQ_GSS 7 /* GSSAPI without wrap() */
-#define AUTH_REQ_GSS_CONT 8 /* Continue GSS exchanges */
-#define AUTH_REQ_SSPI 9 /* SSPI negotiate without wrap() */
-#define AUTH_REQ_SASL 10 /* Begin SASL authentication */
-#define AUTH_REQ_SASL_CONT 11 /* Continue SASL authentication */
-#define AUTH_REQ_SASL_FIN 12 /* Final SASL message */
-#define AUTH_REQ_MAX AUTH_REQ_SASL_FIN /* maximum AUTH_REQ_* value */
-
typedef uint32 AuthRequest;
diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
new file mode 100644
index 0000000000..cc46f4b586
--- /dev/null
+++ b/src/include/libpq/protocol.h
@@ -0,0 +1,85 @@
+/*-------------------------------------------------------------------------
+ *
+ * protocol.h
+ * Definitions of the request/response codes for the wire protocol.
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/libpq/protocol.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PROTOCOL_H
+#define PROTOCOL_H
+
+/* These are the request codes sent by the frontend. */
+
+#define PqMsg_Bind 'B'
+#define PqMsg_Close 'C'
+#define PqMsg_Describe 'D'
+#define PqMsg_Execute 'E'
+#define PqMsg_FunctionCall 'F'
+#define PqMsg_Flush 'H'
+#define PqMsg_Parse 'P'
+#define PqMsg_Query 'Q'
+#define PqMsg_Sync 'S'
+#define PqMsg_Terminate 'X'
+#define PqMsg_CopyFail 'f'
+#define PqMsg_GSSResponse 'p'
+#define PqMsg_PasswordMessage 'p'
+#define PqMsg_SASLInitialResponse 'p'
+#define PqMsg_SASLResponse 'p'
+
+
+/* These are the response codes sent by the backend. */
+
+#define PqMsg_ParseComplete '1'
+#define PqMsg_BindComplete '2'
+#define PqMsg_CloseComplete '3'
+#define PqMsg_NotificationResponse 'A'
+#define PqMsg_CommandComplete 'C'
+#define PqMsg_DataRow 'D'
+#define PqMsg_ErrorResponse 'E'
+#define PqMsg_CopyInResponse 'G'
+#define PqMsg_CopyOutResponse 'H'
+#define PqMsg_EmptyQueryResponse 'I'
+#define PqMsg_BackendKeyData 'K'
+#define PqMsg_NoticeResponse 'N'
+#define PqMsg_AuthenticationRequest 'R'
+#define PqMsg_ParameterStatus 'S'
+#define PqMsg_RowDescription 'T'
+#define PqMsg_FunctionCallResponse 'V'
+#define PqMsg_CopyBothResponse 'W'
+#define PqMsg_ReadyForQuery 'Z'
+#define PqMsg_NoData 'n'
+#define PqMsg_PortalSuspended 's'
+#define PqMsg_ParameterDescription 't'
+#define PqMsg_NegotiateProtocolVersion 'v'
+
+
+/* These are the codes sent by both the frontend and backend. */
+
+#define PqMsg_CopyDone 'c'
+#define PqMsg_CopyData 'd'
+
+
+/* These are the authentication request codes sent by the backend. */
+
+#define AUTH_REQ_OK 0 /* User is authenticated */
+#define AUTH_REQ_KRB4 1 /* Kerberos V4. Not supported any more. */
+#define AUTH_REQ_KRB5 2 /* Kerberos V5. Not supported any more. */
+#define AUTH_REQ_PASSWORD 3 /* Password */
+#define AUTH_REQ_CRYPT 4 /* crypt password. Not supported any more. */
+#define AUTH_REQ_MD5 5 /* md5 password */
+/* 6 is available. It was used for SCM creds, not supported any more. */
+#define AUTH_REQ_GSS 7 /* GSSAPI without wrap() */
+#define AUTH_REQ_GSS_CONT 8 /* Continue GSS exchanges */
+#define AUTH_REQ_SSPI 9 /* SSPI negotiate without wrap() */
+#define AUTH_REQ_SASL 10 /* Begin SASL authentication */
+#define AUTH_REQ_SASL_CONT 11 /* Continue SASL authentication */
+#define AUTH_REQ_SASL_FIN 12 /* Final SASL message */
+#define AUTH_REQ_MAX AUTH_REQ_SASL_FIN /* maximum AUTH_REQ_* value */
+
+#endif /* PROTOCOL_H */
diff --git a/src/include/meson.build b/src/include/meson.build
index d7e1ecd4c9..d50897c9fd 100644
--- a/src/include/meson.build
+++ b/src/include/meson.build
@@ -94,6 +94,7 @@ install_headers(
install_headers(
'libpq/pqcomm.h',
+ 'libpq/protocol.h',
install_dir: dir_include_internal / 'libpq',
)
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 887ca5e9e1..912aa14821 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -586,7 +586,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
/*
* Build a SASLInitialResponse message, and send it.
*/
- if (pqPutMsgStart('p', conn))
+ if (pqPutMsgStart(PqMsg_SASLInitialResponse, conn))
goto error;
if (pqPuts(selected_mechanism, conn))
goto error;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 837c5321aa..bf83a9b569 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3591,7 +3591,9 @@ keep_going: /* We will come back to here until there is
* Anything else probably means it's not Postgres on the other
* end at all.
*/
- if (!(beresp == 'R' || beresp == 'v' || beresp == 'E'))
+ if (beresp != PqMsg_AuthenticationRequest &&
+ beresp != PqMsg_ErrorResponse &&
+ beresp != PqMsg_NegotiateProtocolVersion)
{
libpq_append_conn_error(conn, "expected authentication request from server, but received %c",
beresp);
@@ -3618,19 +3620,22 @@ keep_going: /* We will come back to here until there is
* version 14, the server also used the old protocol for
* errors that happened before processing the startup packet.)
*/
- if (beresp == 'R' && (msgLength < 8 || msgLength > 2000))
+ if (beresp == PqMsg_AuthenticationRequest &&
+ (msgLength < 8 || msgLength > 2000))
{
libpq_append_conn_error(conn, "received invalid authentication request");
goto error_return;
}
- if (beresp == 'v' && (msgLength < 8 || msgLength > 2000))
+ if (beresp == PqMsg_NegotiateProtocolVersion &&
+ (msgLength < 8 || msgLength > 2000))
{
libpq_append_conn_error(conn, "received invalid protocol negotiation message");
goto error_return;
}
#define MAX_ERRLEN 30000
- if (beresp == 'E' && (msgLength < 8 || msgLength > MAX_ERRLEN))
+ if (beresp == PqMsg_ErrorResponse &&
+ (msgLength < 8 || msgLength > MAX_ERRLEN))
{
/* Handle error from a pre-3.0 server */
conn->inCursor = conn->inStart + 1; /* reread data */
@@ -3693,7 +3698,7 @@ keep_going: /* We will come back to here until there is
}
/* Handle errors. */
- if (beresp == 'E')
+ if (beresp == PqMsg_ErrorResponse)
{
if (pqGetErrorNotice3(conn, true))
{
@@ -3770,7 +3775,7 @@ keep_going: /* We will come back to here until there is
goto error_return;
}
- else if (beresp == 'v')
+ else if (beresp == PqMsg_NegotiateProtocolVersion)
{
if (pqGetNegotiateProtocolVersion3(conn))
{
@@ -4540,7 +4545,7 @@ sendTerminateConn(PGconn *conn)
* Try to send "close connection" message to backend. Ignore any
* error.
*/
- pqPutMsgStart('X', conn);
+ pqPutMsgStart(PqMsg_Terminate, conn);
pqPutMsgEnd(conn);
(void) pqFlush(conn);
}
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index a868284ff8..fdb7994779 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1458,7 +1458,7 @@ PQsendQueryInternal(PGconn *conn, const char *query, bool newQuery)
/* Send the query message(s) */
/* construct the outgoing Query message */
- if (pqPutMsgStart('Q', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Query, conn) < 0 ||
pqPuts(query, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
{
@@ -1571,7 +1571,7 @@ PQsendPrepare(PGconn *conn,
return 0; /* error msg already set */
/* construct the Parse message */
- if (pqPutMsgStart('P', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Parse, conn) < 0 ||
pqPuts(stmtName, conn) < 0 ||
pqPuts(query, conn) < 0)
goto sendFailed;
@@ -1599,7 +1599,7 @@ PQsendPrepare(PGconn *conn,
/* Add a Sync, unless in pipeline mode. */
if (conn->pipelineStatus == PQ_PIPELINE_OFF)
{
- if (pqPutMsgStart('S', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Sync, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
goto sendFailed;
}
@@ -1784,7 +1784,7 @@ PQsendQueryGuts(PGconn *conn,
if (command)
{
/* construct the Parse message */
- if (pqPutMsgStart('P', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Parse, conn) < 0 ||
pqPuts(stmtName, conn) < 0 ||
pqPuts(command, conn) < 0)
goto sendFailed;
@@ -1808,7 +1808,7 @@ PQsendQueryGuts(PGconn *conn,
}
/* Construct the Bind message */
- if (pqPutMsgStart('B', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Bind, conn) < 0 ||
pqPuts("", conn) < 0 ||
pqPuts(stmtName, conn) < 0)
goto sendFailed;
@@ -1874,14 +1874,14 @@ PQsendQueryGuts(PGconn *conn,
goto sendFailed;
/* construct the Describe Portal message */
- if (pqPutMsgStart('D', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Describe, conn) < 0 ||
pqPutc('P', conn) < 0 ||
pqPuts("", conn) < 0 ||
pqPutMsgEnd(conn) < 0)
goto sendFailed;
/* construct the Execute message */
- if (pqPutMsgStart('E', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Execute, conn) < 0 ||
pqPuts("", conn) < 0 ||
pqPutInt(0, 4, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
@@ -1890,7 +1890,7 @@ PQsendQueryGuts(PGconn *conn,
/* construct the Sync message if not in pipeline mode */
if (conn->pipelineStatus == PQ_PIPELINE_OFF)
{
- if (pqPutMsgStart('S', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Sync, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
goto sendFailed;
}
@@ -2422,7 +2422,7 @@ PQdescribePrepared(PGconn *conn, const char *stmt)
{
if (!PQexecStart(conn))
return NULL;
- if (!PQsendTypedCommand(conn, 'D', 'S', stmt))
+ if (!PQsendTypedCommand(conn, PqMsg_Describe, 'S', stmt))
return NULL;
return PQexecFinish(conn);
}
@@ -2441,7 +2441,7 @@ PQdescribePortal(PGconn *conn, const char *portal)
{
if (!PQexecStart(conn))
return NULL;
- if (!PQsendTypedCommand(conn, 'D', 'P', portal))
+ if (!PQsendTypedCommand(conn, PqMsg_Describe, 'P', portal))
return NULL;
return PQexecFinish(conn);
}
@@ -2456,7 +2456,7 @@ PQdescribePortal(PGconn *conn, const char *portal)
int
PQsendDescribePrepared(PGconn *conn, const char *stmt)
{
- return PQsendTypedCommand(conn, 'D', 'S', stmt);
+ return PQsendTypedCommand(conn, PqMsg_Describe, 'S', stmt);
}
/*
@@ -2469,7 +2469,7 @@ PQsendDescribePrepared(PGconn *conn, const char *stmt)
int
PQsendDescribePortal(PGconn *conn, const char *portal)
{
- return PQsendTypedCommand(conn, 'D', 'P', portal);
+ return PQsendTypedCommand(conn, PqMsg_Describe, 'P', portal);
}
/*
@@ -2488,7 +2488,7 @@ PQclosePrepared(PGconn *conn, const char *stmt)
{
if (!PQexecStart(conn))
return NULL;
- if (!PQsendTypedCommand(conn, 'C', 'S', stmt))
+ if (!PQsendTypedCommand(conn, PqMsg_Close, 'S', stmt))
return NULL;
return PQexecFinish(conn);
}
@@ -2506,7 +2506,7 @@ PQclosePortal(PGconn *conn, const char *portal)
{
if (!PQexecStart(conn))
return NULL;
- if (!PQsendTypedCommand(conn, 'C', 'P', portal))
+ if (!PQsendTypedCommand(conn, PqMsg_Close, 'P', portal))
return NULL;
return PQexecFinish(conn);
}
@@ -2521,7 +2521,7 @@ PQclosePortal(PGconn *conn, const char *portal)
int
PQsendClosePrepared(PGconn *conn, const char *stmt)
{
- return PQsendTypedCommand(conn, 'C', 'S', stmt);
+ return PQsendTypedCommand(conn, PqMsg_Close, 'S', stmt);
}
/*
@@ -2534,7 +2534,7 @@ PQsendClosePrepared(PGconn *conn, const char *stmt)
int
PQsendClosePortal(PGconn *conn, const char *portal)
{
- return PQsendTypedCommand(conn, 'C', 'P', portal);
+ return PQsendTypedCommand(conn, PqMsg_Close, 'P', portal);
}
/*
@@ -2577,17 +2577,17 @@ PQsendTypedCommand(PGconn *conn, char command, char type, const char *target)
/* construct the Sync message */
if (conn->pipelineStatus == PQ_PIPELINE_OFF)
{
- if (pqPutMsgStart('S', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Sync, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
goto sendFailed;
}
/* remember if we are doing a Close or a Describe */
- if (command == 'C')
+ if (command == PqMsg_Close)
{
entry->queryclass = PGQUERY_CLOSE;
}
- else if (command == 'D')
+ else if (command == PqMsg_Describe)
{
entry->queryclass = PGQUERY_DESCRIBE;
}
@@ -2696,7 +2696,7 @@ PQputCopyData(PGconn *conn, const char *buffer, int nbytes)
return pqIsnonblocking(conn) ? 0 : -1;
}
/* Send the data (too simple to delegate to fe-protocol files) */
- if (pqPutMsgStart('d', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_CopyData, conn) < 0 ||
pqPutnchar(buffer, nbytes, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
return -1;
@@ -2731,7 +2731,7 @@ PQputCopyEnd(PGconn *conn, const char *errormsg)
if (errormsg)
{
/* Send COPY FAIL */
- if (pqPutMsgStart('f', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_CopyFail, conn) < 0 ||
pqPuts(errormsg, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
return -1;
@@ -2739,7 +2739,7 @@ PQputCopyEnd(PGconn *conn, const char *errormsg)
else
{
/* Send COPY DONE */
- if (pqPutMsgStart('c', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_CopyDone, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
return -1;
}
@@ -2751,7 +2751,7 @@ PQputCopyEnd(PGconn *conn, const char *errormsg)
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE)
{
- if (pqPutMsgStart('S', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Sync, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
return -1;
}
@@ -3263,7 +3263,7 @@ PQpipelineSync(PGconn *conn)
entry->query = NULL;
/* construct the Sync message */
- if (pqPutMsgStart('S', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Sync, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
goto sendFailed;
@@ -3311,7 +3311,7 @@ PQsendFlushRequest(PGconn *conn)
return 0;
}
- if (pqPutMsgStart('H', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Flush, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
{
return 0;
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 7bc6355d17..5613c56b14 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -34,8 +34,13 @@
* than a couple of kilobytes).
*/
#define VALID_LONG_MESSAGE_TYPE(id) \
- ((id) == 'T' || (id) == 'D' || (id) == 'd' || (id) == 'V' || \
- (id) == 'E' || (id) == 'N' || (id) == 'A')
+ ((id) == PqMsg_CopyData || \
+ (id) == PqMsg_DataRow || \
+ (id) == PqMsg_ErrorResponse || \
+ (id) == PqMsg_FunctionCallResponse || \
+ (id) == PqMsg_NoticeResponse || \
+ (id) == PqMsg_NotificationResponse || \
+ (id) == PqMsg_RowDescription)
static void handleSyncLoss(PGconn *conn, char id, int msgLength);
@@ -140,12 +145,12 @@ pqParseInput3(PGconn *conn)
* from config file due to SIGHUP), but otherwise we hold off until
* BUSY state.
*/
- if (id == 'A')
+ if (id == PqMsg_NotificationResponse)
{
if (getNotify(conn))
return;
}
- else if (id == 'N')
+ else if (id == PqMsg_NoticeResponse)
{
if (pqGetErrorNotice3(conn, false))
return;
@@ -165,12 +170,12 @@ pqParseInput3(PGconn *conn)
* it is about to close the connection, so we don't want to just
* discard it...)
*/
- if (id == 'E')
+ if (id == PqMsg_ErrorResponse)
{
if (pqGetErrorNotice3(conn, false /* treat as notice */ ))
return;
}
- else if (id == 'S')
+ else if (id == PqMsg_ParameterStatus)
{
if (getParameterStatus(conn))
return;
@@ -192,7 +197,7 @@ pqParseInput3(PGconn *conn)
*/
switch (id)
{
- case 'C': /* command complete */
+ case PqMsg_CommandComplete:
if (pqGets(&conn->workBuffer, conn))
return;
if (!pgHavePendingResult(conn))
@@ -210,13 +215,12 @@ pqParseInput3(PGconn *conn)
CMDSTATUS_LEN);
conn->asyncStatus = PGASYNC_READY;
break;
- case 'E': /* error return */
+ case PqMsg_ErrorResponse:
if (pqGetErrorNotice3(conn, true))
return;
conn->asyncStatus = PGASYNC_READY;
break;
- case 'Z': /* sync response, backend is ready for new
- * query */
+ case PqMsg_ReadyForQuery:
if (getReadyForQuery(conn))
return;
if (conn->pipelineStatus != PQ_PIPELINE_OFF)
@@ -246,7 +250,7 @@ pqParseInput3(PGconn *conn)
conn->asyncStatus = PGASYNC_IDLE;
}
break;
- case 'I': /* empty query */
+ case PqMsg_EmptyQueryResponse:
if (!pgHavePendingResult(conn))
{
conn->result = PQmakeEmptyPGresult(conn,
@@ -259,7 +263,7 @@ pqParseInput3(PGconn *conn)
}
conn->asyncStatus = PGASYNC_READY;
break;
- case '1': /* Parse Complete */
+ case PqMsg_ParseComplete:
/* If we're doing PQprepare, we're done; else ignore */
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass == PGQUERY_PREPARE)
@@ -277,10 +281,10 @@ pqParseInput3(PGconn *conn)
conn->asyncStatus = PGASYNC_READY;
}
break;
- case '2': /* Bind Complete */
+ case PqMsg_BindComplete:
/* Nothing to do for this message type */
break;
- case '3': /* Close Complete */
+ case PqMsg_CloseComplete:
/* If we're doing PQsendClose, we're done; else ignore */
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass == PGQUERY_CLOSE)
@@ -298,11 +302,11 @@ pqParseInput3(PGconn *conn)
conn->asyncStatus = PGASYNC_READY;
}
break;
- case 'S': /* parameter status */
+ case PqMsg_ParameterStatus:
if (getParameterStatus(conn))
return;
break;
- case 'K': /* secret key data from the backend */
+ case PqMsg_BackendKeyData:
/*
* This is expected only during backend startup, but it's
@@ -314,7 +318,7 @@ pqParseInput3(PGconn *conn)
if (pqGetInt(&(conn->be_key), 4, conn))
return;
break;
- case 'T': /* Row Description */
+ case PqMsg_RowDescription:
if (conn->error_result ||
(conn->result != NULL &&
conn->result->resultStatus == PGRES_FATAL_ERROR))
@@ -346,7 +350,7 @@ pqParseInput3(PGconn *conn)
return;
}
break;
- case 'n': /* No Data */
+ case PqMsg_NoData:
/*
* NoData indicates that we will not be seeing a
@@ -374,11 +378,11 @@ pqParseInput3(PGconn *conn)
conn->asyncStatus = PGASYNC_READY;
}
break;
- case 't': /* Parameter Description */
+ case PqMsg_ParameterDescription:
if (getParamDescriptions(conn, msgLength))
return;
break;
- case 'D': /* Data Row */
+ case PqMsg_DataRow:
if (conn->result != NULL &&
conn->result->resultStatus == PGRES_TUPLES_OK)
{
@@ -405,24 +409,24 @@ pqParseInput3(PGconn *conn)
conn->inCursor += msgLength;
}
break;
- case 'G': /* Start Copy In */
+ case PqMsg_CopyInResponse:
if (getCopyStart(conn, PGRES_COPY_IN))
return;
conn->asyncStatus = PGASYNC_COPY_IN;
break;
- case 'H': /* Start Copy Out */
+ case PqMsg_CopyOutResponse:
if (getCopyStart(conn, PGRES_COPY_OUT))
return;
conn->asyncStatus = PGASYNC_COPY_OUT;
conn->copy_already_done = 0;
break;
- case 'W': /* Start Copy Both */
+ case PqMsg_CopyBothResponse:
if (getCopyStart(conn, PGRES_COPY_BOTH))
return;
conn->asyncStatus = PGASYNC_COPY_BOTH;
conn->copy_already_done = 0;
break;
- case 'd': /* Copy Data */
+ case PqMsg_CopyData:
/*
* If we see Copy Data, just silently drop it. This would
@@ -431,7 +435,7 @@ pqParseInput3(PGconn *conn)
*/
conn->inCursor += msgLength;
break;
- case 'c': /* Copy Done */
+ case PqMsg_CopyDone:
/*
* If we see Copy Done, just silently drop it. This is
@@ -1692,21 +1696,21 @@ getCopyDataMessage(PGconn *conn)
*/
switch (id)
{
- case 'A': /* NOTIFY */
+ case PqMsg_NotificationResponse:
if (getNotify(conn))
return 0;
break;
- case 'N': /* NOTICE */
+ case PqMsg_NoticeResponse:
if (pqGetErrorNotice3(conn, false))
return 0;
break;
- case 'S': /* ParameterStatus */
+ case PqMsg_ParameterStatus:
if (getParameterStatus(conn))
return 0;
break;
- case 'd': /* Copy Data, pass it back to caller */
+ case PqMsg_CopyData:
return msgLength;
- case 'c':
+ case PqMsg_CopyDone:
/*
* If this is a CopyDone message, exit COPY_OUT mode and let
@@ -1929,7 +1933,7 @@ pqEndcopy3(PGconn *conn)
if (conn->asyncStatus == PGASYNC_COPY_IN ||
conn->asyncStatus == PGASYNC_COPY_BOTH)
{
- if (pqPutMsgStart('c', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_CopyDone, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
return 1;
@@ -1940,7 +1944,7 @@ pqEndcopy3(PGconn *conn)
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE)
{
- if (pqPutMsgStart('S', conn) < 0 ||
+ if (pqPutMsgStart(PqMsg_Sync, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
return 1;
}
@@ -2023,7 +2027,7 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
/* PQfn already validated connection state */
- if (pqPutMsgStart('F', conn) < 0 || /* function call msg */
+ if (pqPutMsgStart(PqMsg_FunctionCall, conn) < 0 ||
pqPutInt(fnid, 4, conn) < 0 || /* function id */
pqPutInt(1, 2, conn) < 0 || /* # of format codes */
pqPutInt(1, 2, conn) < 0 || /* format code: BINARY */
diff --git a/src/interfaces/libpq/fe-trace.c b/src/interfaces/libpq/fe-trace.c
index 402784f40e..b18e3deab6 100644
--- a/src/interfaces/libpq/fe-trace.c
+++ b/src/interfaces/libpq/fe-trace.c
@@ -562,110 +562,120 @@ pqTraceOutputMessage(PGconn *conn, const char *message, bool toServer)
switch (id)
{
- case '1':
+ case PqMsg_ParseComplete:
fprintf(conn->Pfdebug, "ParseComplete");
/* No message content */
break;
- case '2':
+ case PqMsg_BindComplete:
fprintf(conn->Pfdebug, "BindComplete");
/* No message content */
break;
- case '3':
+ case PqMsg_CloseComplete:
fprintf(conn->Pfdebug, "CloseComplete");
/* No message content */
break;
- case 'A': /* Notification Response */
+ case PqMsg_NotificationResponse:
pqTraceOutputA(conn->Pfdebug, message, &logCursor, regress);
break;
- case 'B': /* Bind */
+ case PqMsg_Bind:
pqTraceOutputB(conn->Pfdebug, message, &logCursor);
break;
- case 'c':
+ case PqMsg_CopyDone:
fprintf(conn->Pfdebug, "CopyDone");
/* No message content */
break;
- case 'C': /* Close(F) or Command Complete(B) */
+ case PqMsg_CommandComplete:
+ /* Close(F) and CommandComplete(B) use the same identifier. */
+ Assert(PqMsg_Close == PqMsg_CommandComplete);
pqTraceOutputC(conn->Pfdebug, toServer, message, &logCursor);
break;
- case 'd': /* Copy Data */
+ case PqMsg_CopyData:
/* Drop COPY data to reduce the overhead of logging. */
break;
- case 'D': /* Describe(F) or Data Row(B) */
+ case PqMsg_Describe:
+ /* Describe(F) and DataRow(B) use the same identifier. */
+ Assert(PqMsg_Describe == PqMsg_DataRow);
pqTraceOutputD(conn->Pfdebug, toServer, message, &logCursor);
break;
- case 'E': /* Execute(F) or Error Response(B) */
+ case PqMsg_Execute:
+ /* Execute(F) and ErrorResponse(B) use the same identifier. */
+ Assert(PqMsg_Execute == PqMsg_ErrorResponse);
pqTraceOutputE(conn->Pfdebug, toServer, message, &logCursor,
regress);
break;
- case 'f': /* Copy Fail */
+ case PqMsg_CopyFail:
pqTraceOutputf(conn->Pfdebug, message, &logCursor);
break;
- case 'F': /* Function Call */
+ case PqMsg_FunctionCall:
pqTraceOutputF(conn->Pfdebug, message, &logCursor, regress);
break;
- case 'G': /* Start Copy In */
+ case PqMsg_CopyInResponse:
pqTraceOutputG(conn->Pfdebug, message, &logCursor);
break;
- case 'H': /* Flush(F) or Start Copy Out(B) */
+ case PqMsg_Flush:
+ /* Flush(F) and CopyOutResponse(B) use the same identifier */
+ Assert(PqMsg_CopyOutResponse == PqMsg_Flush);
if (!toServer)
pqTraceOutputH(conn->Pfdebug, message, &logCursor);
else
fprintf(conn->Pfdebug, "Flush"); /* no message content */
break;
- case 'I':
+ case PqMsg_EmptyQueryResponse:
fprintf(conn->Pfdebug, "EmptyQueryResponse");
/* No message content */
break;
- case 'K': /* secret key data from the backend */
+ case PqMsg_BackendKeyData:
pqTraceOutputK(conn->Pfdebug, message, &logCursor, regress);
break;
- case 'n':
+ case PqMsg_NoData:
fprintf(conn->Pfdebug, "NoData");
/* No message content */
break;
- case 'N':
+ case PqMsg_NoticeResponse:
pqTraceOutputNR(conn->Pfdebug, "NoticeResponse", message,
&logCursor, regress);
break;
- case 'P': /* Parse */
+ case PqMsg_Parse:
pqTraceOutputP(conn->Pfdebug, message, &logCursor, regress);
break;
- case 'Q': /* Query */
+ case PqMsg_Query:
pqTraceOutputQ(conn->Pfdebug, message, &logCursor);
break;
- case 'R': /* Authentication */
+ case PqMsg_AuthenticationRequest:
pqTraceOutputR(conn->Pfdebug, message, &logCursor);
break;
- case 's':
+ case PqMsg_PortalSuspended:
fprintf(conn->Pfdebug, "PortalSuspended");
/* No message content */
break;
- case 'S': /* Parameter Status(B) or Sync(F) */
+ case PqMsg_Sync:
+ /* Parameter Status(B) and Sync(F) use the same identifier */
+ Assert(PqMsg_ParameterStatus == PqMsg_Sync);
if (!toServer)
pqTraceOutputS(conn->Pfdebug, message, &logCursor);
else
fprintf(conn->Pfdebug, "Sync"); /* no message content */
break;
- case 't': /* Parameter Description */
+ case PqMsg_ParameterDescription:
pqTraceOutputt(conn->Pfdebug, message, &logCursor, regress);
break;
- case 'T': /* Row Description */
+ case PqMsg_RowDescription:
pqTraceOutputT(conn->Pfdebug, message, &logCursor, regress);
break;
- case 'v': /* Negotiate Protocol Version */
+ case PqMsg_NegotiateProtocolVersion:
pqTraceOutputv(conn->Pfdebug, message, &logCursor);
break;
- case 'V': /* Function Call response */
+ case PqMsg_FunctionCallResponse:
pqTraceOutputV(conn->Pfdebug, message, &logCursor);
break;
- case 'W': /* Start Copy Both */
+ case PqMsg_CopyBothResponse:
pqTraceOutputW(conn->Pfdebug, message, &logCursor, length);
break;
- case 'X':
+ case PqMsg_Terminate:
fprintf(conn->Pfdebug, "Terminate");
/* No message content */
break;
- case 'Z': /* Ready For Query */
+ case PqMsg_ReadyForQuery:
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;
default:
--
2.25.1
--GvXjxJ+pjyke8COw--
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-08-28 21:49 Tristen Raab <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Tristen Raab @ 2023-08-28 21:49 UTC (permalink / raw)
To: [email protected]; +Cc: Jelte Fennema <[email protected]>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hello,
I've reviewed your patch and it applies and builds without error. When testing this patch I was slightly confused as to what its purpose was, after testing it I now understand. Initially, I thought this was a change to add database-level replication. I would suggest some clarifications to the documentation such as changing:
"supplying a specific database name in the connection string won't cause PostgreSQL to behave any differently."
to
"supplying a specific database name in the connection string won't cause pg_basebackup to behave any differently."
I believe this better illustrates that we are referring to the actual pg_basebackup utility and how this parameter is only used for proxies and bears no impact on what pg_basebackup is actually doing. It also would remove any confusion about database replication I had prior.
There is also a small typo in the same documentation:
"However, if you are connecting to PostgreSQL through a proxy, then it's possible that this proxy does use the supplied databasename to make certain decisions, such as to which cluster to route the connection."
"databasename" is just missing a space.
Other than that, everything looks good.
Regards,
Tristen
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-08-29 13:55 Jelte Fennema <[email protected]>
parent: Tristen Raab <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jelte Fennema @ 2023-08-29 13:55 UTC (permalink / raw)
To: Tristen Raab <[email protected]>; +Cc: [email protected]; Jelte Fennema <[email protected]>
On Mon, 28 Aug 2023 at 23:50, Tristen Raab <[email protected]> wrote:
> I've reviewed your patch and it applies and builds without error. When testing this patch I was slightly confused as to what its purpose was, after testing it I now understand. Initially, I thought this was a change to add database-level replication. I would suggest some clarifications to the documentation such as changing:
Thanks for the review. I've updated the documentation to make it
clearer (using some of your suggestions but also some others)
Attachments:
[application/octet-stream] v3-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch (6.1K, ../../CAGECzQTEXbhQYOhxWAiGWqc-Ec_av1qi688kyDTkpyALUidSVQ@mail.gmail.com/2-v3-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch)
download | inline diff:
From 34258f0f6ad2410bfec9d1a398d121ce466f79a2 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Mon, 3 Jul 2023 13:50:31 +0200
Subject: [PATCH v3] Allow specifying a dbname in pg_basebackup connection
string
Normally it doesn't really matter which dbname is used in the connection
string that pg_basebackup and other physical replication CLI tools use.
The reason being, that physical replication does not work at the
database level, but instead at the server level. So you will always get
the data for all databases.
However, when there's a proxy, such as PgBouncer, in between the client
and the server, then might very well matter. Because this proxy might
want to route the connection to a different server depending on the
dbname parameter in the startup packet.
This patch changes the creation of the connection string key value
pairs, so that the following command will actually include
dbname=postgres in the startup packet to the server:
```
pg_basebackup --dbname 'dbname=postgres port=6432'
```
This also applies to other physical replication CLI tools like
pg_receivewal.
---
doc/src/sgml/ref/pg_basebackup.sgml | 11 +++++++++--
doc/src/sgml/ref/pg_receivewal.sgml | 11 +++++++++--
src/bin/pg_basebackup/streamutil.c | 29 ++++++++++++++++++-----------
3 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 79d3e657c32..88d20abe723 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -752,8 +752,15 @@ PostgreSQL documentation
<para>
The option is called <literal>--dbname</literal> for consistency with other
client applications, but because <application>pg_basebackup</application>
- doesn't connect to any particular database in the cluster, any database
- name in the connection string will be ignored.
+ doesn't connect to any particular database in the cluster, supplying a
+ specific database name in the connection string won't cause
+ <application>pg_basebackup</application> to behave any
+ differently when connecting directly to a
+ <productname>PostgreSQL</productname> server.
+ However, if you are connecting to <productname>PostgreSQL</productname>
+ through a proxy, then it's possible that this proxy does use the
+ supplied databasename to make certain decisions, such as to which
+ cluster to route the connection.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index cecc7daec97..5e3ff238159 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -316,8 +316,15 @@ PostgreSQL documentation
<para>
The option is called <literal>--dbname</literal> for consistency with other
client applications, but because <application>pg_receivewal</application>
- doesn't connect to any particular database in the cluster, database
- name in the connection string will be ignored.
+ doesn't connect to any particular database in the cluster, supplying a
+ specific database name in the connection string won't cause
+ <application>pg_receivewal</application> to behave any
+ differently when connecting directly to a
+ <productname>PostgreSQL</productname> server.
+ However, if you are connecting to <productname>PostgreSQL</productname>
+ through a proxy, then it's possible that this proxy does use the
+ supplied database name to make certain decisions, such as to which
+ cluster to route the connection.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 75ab9e56f3e..dbd08ab1722 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -79,9 +79,6 @@ GetConnection(void)
/*
* Merge the connection info inputs given in form of connection string,
* options and default values (dbname=replication, replication=true, etc.)
- * Explicitly discard any dbname value in the connection string;
- * otherwise, PQconnectdbParams() would interpret that value as being
- * itself a connection string.
*/
i = 0;
if (connection_string)
@@ -92,18 +89,24 @@ GetConnection(void)
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
argcount++;
}
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ /*
+ * Set dbname here already, so it can be overridden by a dbname in the
+ * connection string.
+ */
+ keywords[i] = "dbname";
+ values[i] = "replication";
+ i++;
+
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
{
keywords[i] = conn_opt->keyword;
values[i] = conn_opt->val;
@@ -115,11 +118,11 @@ GetConnection(void)
{
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ keywords[i] = "dbname";
+ values[i] = dbname;
+ i++;
}
- keywords[i] = "dbname";
- values[i] = dbname == NULL ? "replication" : dbname;
- i++;
keywords[i] = "replication";
values[i] = dbname == NULL ? "true" : "database";
i++;
@@ -171,7 +174,11 @@ GetConnection(void)
values[i] = NULL;
}
- tmpconn = PQconnectdbParams(keywords, values, true);
+ /*
+ * Only expand dbname when we did not already parse the argument as a
+ * connection string ourselves.
+ */
+ tmpconn = PQconnectdbParams(keywords, values, !connection_string);
/*
* If there is too little memory even to allocate the PGconn object
base-commit: 63956bed7b10fb3bb5fe3f74250a33c9e226a921
--
2.34.1
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-08-29 23:01 Jim Jones <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jim Jones @ 2023-08-29 23:01 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; Tristen Raab <[email protected]>; +Cc: [email protected]; Jelte Fennema <[email protected]>
Hi Jelte
On 29.08.23 15:55, Jelte Fennema wrote:
> Thanks for the review. I've updated the documentation to make it
> clearer (using some of your suggestions but also some others)
This patch applies and builds cleanly, and the documentation is very clear.
I tested it using the 'replication-support' branch from your github fork:
/pg_basebackup --dbname "port=6432 user=postgres dbname=foo" -D /tmp/dump1/
pgbouncer log:
/2023-08-30 00:50:52.866 CEST [811770] LOG C-0x555fbd65bf40:
(nodb)/postgres@unix(811776):6432 login attempt: db=foo user=postgres
tls=no replication=yes/
However, pgbouncer closes with a segmentation fault, so I couldn't test
the result of pg_basebackup itself - but I guess it isn't the issue here.
Other than that, everything looks good to me.
Jim
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-08-30 12:11 Jelte Fennema <[email protected]>
parent: Jim Jones <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jelte Fennema @ 2023-08-30 12:11 UTC (permalink / raw)
To: Jim Jones <[email protected]>; +Cc: Tristen Raab <[email protected]>; [email protected]; Jelte Fennema <[email protected]>
On Wed, 30 Aug 2023 at 01:01, Jim Jones <[email protected]> wrote:
> However, pgbouncer closes with a segmentation fault, so I couldn't test the result of pg_basebackup itself - but I guess it isn't the issue here.
Oops it indeed seemed like I made an unintended change when handling
database names that did not exist in pgbouncer.conf when you used
auth_type=hba. I pushed a fix for that now to the replication-support
branch. Feel free to try again. But as you said it's unrelated to the
postgres patch.
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-08-30 19:08 Jim Jones <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jim Jones @ 2023-08-30 19:08 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: Tristen Raab <[email protected]>; [email protected]; Jelte Fennema <[email protected]>
On 30.08.23 14:11, Jelte Fennema wrote:
> Oops it indeed seemed like I made an unintended change when handling
> database names that did not exist in pgbouncer.conf when you used
> auth_type=hba. I pushed a fix for that now to the replication-support
> branch. Feel free to try again. But as you said it's unrelated to the
> postgres patch.
Nice! Now it seems to work as expected :)
$ /usr/local/postgres-dev/bin/pg_basebackup --dbname "host=127.0.0.1
port=6432 user=jim dbname=foo" -X fetch -v -l testlabel -D /tmp/dump
pg_basebackup: initiating base backup, waiting for checkpoint to complete
pg_basebackup: checkpoint completed
pg_basebackup: write-ahead log start point: 0/12000028 on timeline 1
pg_basebackup: write-ahead log end point: 0/12000100
pg_basebackup: syncing data to disk ...
pg_basebackup: renaming backup_manifest.tmp to backup_manifest
pg_basebackup: base backup completed
pgbouncer log:
2023-08-30 21:04:30.225 CEST [785989] LOG C-0x55fbee0f50b0:
foo/[email protected]:49764 login attempt: db=foo user=jim tls=no
replication=yes
2023-08-30 21:04:30.225 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 new connection to server (from 127.0.0.1:34400)
2023-08-30 21:04:30.226 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 closing because: replication client was closed
(age=0s)
2023-08-30 21:04:30.226 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 new connection to server (from 127.0.0.1:34408)
2023-08-30 21:04:30.227 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 closing because: replication client was closed
(age=0s)
2023-08-30 21:04:30.227 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 new connection to server (from 127.0.0.1:34410)
2023-08-30 21:04:30.228 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 closing because: replication client was closed
(age=0s)
2023-08-30 21:04:30.228 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 new connection to server (from 127.0.0.1:34418)
2023-08-30 21:04:30.309 CEST [785989] LOG S-0x55fbee0fc560:
foo/[email protected]:5432 closing because: replication client was closed
(age=0s)
2023-08-30 21:04:30.309 CEST [785989] LOG C-0x55fbee0f50b0:
foo/[email protected]:49764 closing because: client close request (age=0s)
Jim
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-08-31 09:01 Jelte Fennema <[email protected]>
parent: Jim Jones <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jelte Fennema @ 2023-08-31 09:01 UTC (permalink / raw)
To: Jim Jones <[email protected]>; +Cc: Tristen Raab <[email protected]>; [email protected]; Jelte Fennema <[email protected]>
Attached is a new version with some slightly updated wording in the docs
Attachments:
[application/octet-stream] v4-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch (6.2K, ../../CAGECzQRoJ2mCbBBtm5wiFrgxg91gf4T2DGETB+WpWN071thxig@mail.gmail.com/2-v4-0001-Allow-specifying-a-dbname-in-pg_basebackup-connec.patch)
download | inline diff:
From d1b6daf048781ab94386b580253c02fc1d8938aa Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Mon, 3 Jul 2023 13:50:31 +0200
Subject: [PATCH v4] Allow specifying a dbname in pg_basebackup connection
string
Normally it doesn't really matter which dbname is used in the connection
string that pg_basebackup and other physical replication CLI tools use.
The reason being, that physical replication does not work at the
database level, but instead at the server level. So you will always get
the data for all databases.
However, when there's a proxy, such as PgBouncer, in between the client
and the server, then might very well matter. Because this proxy might
want to route the connection to a different server depending on the
dbname parameter in the startup packet.
This patch changes the creation of the connection string key value
pairs, so that the following command will actually include
dbname=postgres in the startup packet to the server:
```
pg_basebackup --dbname 'dbname=postgres port=6432'
```
This also applies to other physical replication CLI tools like
pg_receivewal.
---
doc/src/sgml/ref/pg_basebackup.sgml | 12 ++++++++++--
doc/src/sgml/ref/pg_receivewal.sgml | 12 ++++++++++--
src/bin/pg_basebackup/streamutil.c | 29 ++++++++++++++++++-----------
3 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 79d3e657c32..c5e74bc83ab 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -752,8 +752,16 @@ PostgreSQL documentation
<para>
The option is called <literal>--dbname</literal> for consistency with other
client applications, but because <application>pg_basebackup</application>
- doesn't connect to any particular database in the cluster, any database
- name in the connection string will be ignored.
+ doesn't connect to any particular database in the cluster, supplying a
+ specific database name in the connection string won't cause
+ <application>pg_basebackup</application> to behave any
+ differently when connecting directly to a
+ <productname>PostgreSQL</productname> server.
+ However, if you are connecting to <productname>PostgreSQL</productname>
+ through a proxy, then it's possible that this proxy does use the
+ supplied database name to make certain decisions, such as to which
+ database cluster to route the connection if the proxy can connect to
+ multiple different clusters.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index cecc7daec97..2f152e645b0 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -316,8 +316,16 @@ PostgreSQL documentation
<para>
The option is called <literal>--dbname</literal> for consistency with other
client applications, but because <application>pg_receivewal</application>
- doesn't connect to any particular database in the cluster, database
- name in the connection string will be ignored.
+ doesn't connect to any particular database in the cluster, supplying a
+ specific database name in the connection string won't cause
+ <application>pg_receivewal</application> to behave any
+ differently when connecting directly to a
+ <productname>PostgreSQL</productname> server.
+ However, if you are connecting to <productname>PostgreSQL</productname>
+ through a proxy, then it's possible that this proxy does use the
+ supplied database name to make certain decisions, such as to which
+ database cluster to route the connection if the proxy can connect to
+ multiple different clusters.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 75ab9e56f3e..dbd08ab1722 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -79,9 +79,6 @@ GetConnection(void)
/*
* Merge the connection info inputs given in form of connection string,
* options and default values (dbname=replication, replication=true, etc.)
- * Explicitly discard any dbname value in the connection string;
- * otherwise, PQconnectdbParams() would interpret that value as being
- * itself a connection string.
*/
i = 0;
if (connection_string)
@@ -92,18 +89,24 @@ GetConnection(void)
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
argcount++;
}
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ /*
+ * Set dbname here already, so it can be overridden by a dbname in the
+ * connection string.
+ */
+ keywords[i] = "dbname";
+ values[i] = "replication";
+ i++;
+
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
{
- if (conn_opt->val != NULL && conn_opt->val[0] != '\0' &&
- strcmp(conn_opt->keyword, "dbname") != 0)
+ if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
{
keywords[i] = conn_opt->keyword;
values[i] = conn_opt->val;
@@ -115,11 +118,11 @@ GetConnection(void)
{
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
+ keywords[i] = "dbname";
+ values[i] = dbname;
+ i++;
}
- keywords[i] = "dbname";
- values[i] = dbname == NULL ? "replication" : dbname;
- i++;
keywords[i] = "replication";
values[i] = dbname == NULL ? "true" : "database";
i++;
@@ -171,7 +174,11 @@ GetConnection(void)
values[i] = NULL;
}
- tmpconn = PQconnectdbParams(keywords, values, true);
+ /*
+ * Only expand dbname when we did not already parse the argument as a
+ * connection string ourselves.
+ */
+ tmpconn = PQconnectdbParams(keywords, values, !connection_string);
/*
* If there is too little memory even to allocate the PGconn object
base-commit: 63956bed7b10fb3bb5fe3f74250a33c9e226a921
--
2.34.1
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-09-18 12:11 Daniel Gustafsson <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Gustafsson @ 2023-09-18 12:11 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: Jim Jones <[email protected]>; Tristen Raab <[email protected]>; [email protected]; Jelte Fennema <[email protected]>
> On 31 Aug 2023, at 11:01, Jelte Fennema <[email protected]> wrote:
> Attached is a new version with some slightly updated wording in the docs
I had a look at this Ready for Committer entry in the CF and it seems to strike
a balance between useful in certain cases and non-intrusive in others.
Unless something sticks out in a second pass over it I will go ahead and apply
it.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2023-09-21 12:53 Daniel Gustafsson <[email protected]>
parent: Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Daniel Gustafsson @ 2023-09-21 12:53 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: Jim Jones <[email protected]>; Tristen Raab <[email protected]>; PostgreSQL Hackers <[email protected]>; Jelte Fennema <[email protected]>
> On 18 Sep 2023, at 14:11, Daniel Gustafsson <[email protected]> wrote:
> Unless something sticks out in a second pass over it I will go ahead and apply
> it.
And applied, closing the CF entry.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2024-11-04 17:14 Tom Lane <[email protected]>
parent: Daniel Gustafsson <[email protected]>
0 siblings, 2 replies; 36+ messages in thread
From: Tom Lane @ 2024-11-04 17:14 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Jim Jones <[email protected]>; Tristen Raab <[email protected]>; PostgreSQL Hackers <[email protected]>; Jelte Fennema <[email protected]>
Daniel Gustafsson <[email protected]> writes:
> On 18 Sep 2023, at 14:11, Daniel Gustafsson <[email protected]> wrote:
>> Unless something sticks out in a second pass over it I will go ahead and apply
>> it.
> And applied, closing the CF entry.
I believe this patch (commit cca97ce6a) is the cause of bug #18685,
which reports that pg_basebackup fails to use ~/.pgpass for
connections [1]. Specifically, it replaced this code:
- keywords[i] = "dbname";
- values[i] = dbname == NULL ? "replication" : dbname;
with this code:
+ keywords[i] = "dbname";
+ values[i] = dbname;
perhaps under the illusion that dbname and connection_string can't
both be NULL. But that ends in passing no dbname to libpq, rather
than passing "replication" which is the correct thing. Apparently
that has no effect on the server connection, but it does break
matching of such connections to ~/.pgpass.
I think the attached will fix it, but I wonder if there are edge
cases I'm not thinking of.
regards, tom lane
[1] https://www.postgresql.org/message-id/18685-fee2dd142b9688f1%40postgresql.org
Attachments:
[text/x-diff] fix-pg_basebackup-pgpass-matching.patch (485B, ../../[email protected]/2-fix-pg_basebackup-pgpass-matching.patch)
download | inline diff:
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 30b3d9a377..d074b5ddd7 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -120,7 +120,7 @@ GetConnection(void)
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
keywords[i] = "dbname";
- values[i] = dbname;
+ values[i] = (dbname == NULL) ? "replication" : dbname;
i++;
}
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2024-11-04 18:20 Jelte Fennema-Nio <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 0 replies; 36+ messages in thread
From: Jelte Fennema-Nio @ 2024-11-04 18:20 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Jim Jones <[email protected]>; Tristen Raab <[email protected]>; PostgreSQL Hackers <[email protected]>; Jelte Fennema <[email protected]>
On Mon, 4 Nov 2024 at 18:14, Tom Lane <[email protected]> wrote:
> perhaps under the illusion that dbname and connection_string can't
> both be NULL.
Yeah, I'm pretty sure I didn't realise that was an option. I think I
probably misinterpreted this comment (might be nice to clarify there
that both being NULL is also a valid option):
/* pg_recvlogical uses dbname only; others use connection_string only. */
Assert(dbname == NULL || connection_string == NULL);
> I think the attached will fix it, but I wonder if there are edge
> cases I'm not thinking of.
Yeah that patch looks good to me. Reading the patch carefully again I
cannot think there are other changes in behaviour (except the ones
intended by the change).
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2024-11-04 18:34 Daniel Gustafsson <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: Daniel Gustafsson @ 2024-11-04 18:34 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Jim Jones <[email protected]>; Tristen Raab <[email protected]>; PostgreSQL Hackers <[email protected]>; Jelte Fennema <[email protected]>
> On 4 Nov 2024, at 18:14, Tom Lane <[email protected]> wrote:
> perhaps under the illusion that dbname and connection_string can't
> both be NULL.
Ugh, yes, I failed to capture that nuance.
> I think the attached will fix it, but I wonder if there are edge
> cases I'm not thinking of.
+1 on your patch, reading through I can't see anything it misses. Maybe a
comment above the change with a note on why dbname is tested for NULL there
would be good?
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2024-11-04 18:57 Tom Lane <[email protected]>
parent: Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Tom Lane @ 2024-11-04 18:57 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Jelte Fennema <[email protected]>; Jim Jones <[email protected]>; Tristen Raab <[email protected]>; PostgreSQL Hackers <[email protected]>; Jelte Fennema <[email protected]>
Daniel Gustafsson <[email protected]> writes:
> +1 on your patch, reading through I can't see anything it misses. Maybe a
> comment above the change with a note on why dbname is tested for NULL there
> would be good?
I thought Jelte's suggestion of clarifying the first comment was good:
- /* pg_recvlogical uses dbname only; others use connection_string only. */
+ /*
+ * pg_recvlogical uses dbname only; others use connection_string only.
+ * (Note: both variables will be NULL if there's no command line options.)
+ */
Assert(dbname == NULL || connection_string == NULL);
Also, I realized that the associated documentation is pretty much a
lie:
... any database
name in the connection string will be ignored
by <productname>PostgreSQL</productname>.
Maybe that's true for a very narrow interpretation of "PostgreSQL",
but I think most people would consider that pg_basebackup's lookup
of ~/.pgpass entries is covered. I'm intending to go with the
attached.
regards, tom lane
Attachments:
[text/x-diff] v2-fix-pg_basebackup-pgpass-matching.patch (2.8K, ../../[email protected]/2-v2-fix-pg_basebackup-pgpass-matching.patch)
download | inline diff:
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 4f99340c1d..c2d721208b 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -807,14 +807,18 @@ PostgreSQL documentation
will override any conflicting command line options.
</para>
<para>
- The option is called <literal>--dbname</literal> for consistency with other
+ This option is called <literal>--dbname</literal> for consistency with other
client applications, but because <application>pg_basebackup</application>
doesn't connect to any particular database in the cluster, any database
- name in the connection string will be ignored
- by <productname>PostgreSQL</productname>. Middleware, or proxies, used in
- connecting to <productname>PostgreSQL</productname> might however
- utilize the value. The database name specified in connection string can
- also be used by <link linkend="logicaldecoding-replication-slots-synchronization">
+ name included in the connection string will be ignored by the server.
+ However, a database name supplied that way overrides the default
+ database name (<literal>replication</literal>) for purposes of
+ looking up the replication connection's password
+ in <filename>~/.pgpass</filename>. Similarly, middleware or proxies
+ used in connecting to <productname>PostgreSQL</productname> might
+ utilize the name for purposes such as connection routing. The
+ database name can also be used
+ by <link linkend="logicaldecoding-replication-slots-synchronization">
logical replication slot synchronization</link>.
</para>
</listitem>
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 30b3d9a377..9b86482546 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -74,7 +74,10 @@ GetConnection(void)
PQconninfoOption *conn_opt;
char *err_msg = NULL;
- /* pg_recvlogical uses dbname only; others use connection_string only. */
+ /*
+ * pg_recvlogical uses dbname only; others use connection_string only.
+ * (Note: both variables will be NULL if there's no command line options.)
+ */
Assert(dbname == NULL || connection_string == NULL);
/*
@@ -120,12 +123,12 @@ GetConnection(void)
keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
values = pg_malloc0((argcount + 1) * sizeof(*values));
keywords[i] = "dbname";
- values[i] = dbname;
+ values[i] = (dbname == NULL) ? "replication" : dbname;
i++;
}
keywords[i] = "replication";
- values[i] = dbname == NULL ? "true" : "database";
+ values[i] = (dbname == NULL) ? "true" : "database";
i++;
keywords[i] = "fallback_application_name";
values[i] = progname;
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2024-11-04 19:09 Jelte Fennema-Nio <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Jelte Fennema-Nio @ 2024-11-04 19:09 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Jim Jones <[email protected]>; Tristen Raab <[email protected]>; PostgreSQL Hackers <[email protected]>; Jelte Fennema <[email protected]>
On Mon, 4 Nov 2024 at 19:57, Tom Lane <[email protected]> wrote:
> I'm intending to go with the attached.
The new documentation looks good to me, but I think the same change
should be applied to the documentation for pg_receivewal.
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: Allow specifying a dbname in pg_basebackup connection string
@ 2024-11-04 19:18 Tom Lane <[email protected]>
parent: Jelte Fennema-Nio <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Tom Lane @ 2024-11-04 19:18 UTC (permalink / raw)
To: Jelte Fennema-Nio <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Jim Jones <[email protected]>; Tristen Raab <[email protected]>; PostgreSQL Hackers <[email protected]>; Jelte Fennema <[email protected]>
Jelte Fennema-Nio <[email protected]> writes:
> The new documentation looks good to me, but I think the same change
> should be applied to the documentation for pg_receivewal.
Good idea, will do.
regards, tom lane
^ permalink raw reply [nested|flat] 36+ messages in thread
end of thread, other threads:[~2024-11-04 19:18 UTC | newest]
Thread overview: 36+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-05-03 14:24 [PATCH v3] make \d pg_toast.foo show its indices Justin Pryzby <[email protected]>
2019-05-03 14:24 [PATCH v5] make \d pg_toast.foo show its indices Justin Pryzby <[email protected]>
2019-05-03 14:24 [PATCH v3] make \d pg_toast.foo show its indices Justin Pryzby <[email protected]>
2019-05-03 14:24 [PATCH v7] make \d pg_toast.foo show its indices Justin Pryzby <[email protected]>
2019-05-03 14:24 [PATCH v2 1/2] make \d pg_toast.foo show its indices Justin Pryzby <[email protected]>
2019-05-03 14:24 [PATCH v5] make \d pg_toast.foo show its indices Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH v8 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH v4 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2021-12-17 15:35 [PATCH 4/4] Move the double-plus "Size" columns to the right Justin Pryzby <[email protected]>
2023-07-03 12:22 Allow specifying a dbname in pg_basebackup connection string Jelte Fennema <[email protected]>
2023-07-05 12:43 ` Re: Allow specifying a dbname in pg_basebackup connection string Thom Brown <[email protected]>
2023-07-05 14:01 ` Re: Allow specifying a dbname in pg_basebackup connection string Euler Taveira <[email protected]>
2023-07-05 15:50 ` Re: Allow specifying a dbname in pg_basebackup connection string Jelte Fennema <[email protected]>
2023-07-05 18:08 ` Re: Allow specifying a dbname in pg_basebackup connection string Thom Brown <[email protected]>
2023-07-05 19:39 ` Re: Allow specifying a dbname in pg_basebackup connection string Jelte Fennema <[email protected]>
2023-08-28 21:49 ` Re: Allow specifying a dbname in pg_basebackup connection string Tristen Raab <[email protected]>
2023-08-29 13:55 ` Re: Allow specifying a dbname in pg_basebackup connection string Jelte Fennema <[email protected]>
2023-08-29 23:01 ` Re: Allow specifying a dbname in pg_basebackup connection string Jim Jones <[email protected]>
2023-08-30 12:11 ` Re: Allow specifying a dbname in pg_basebackup connection string Jelte Fennema <[email protected]>
2023-08-30 19:08 ` Re: Allow specifying a dbname in pg_basebackup connection string Jim Jones <[email protected]>
2023-08-31 09:01 ` Re: Allow specifying a dbname in pg_basebackup connection string Jelte Fennema <[email protected]>
2023-09-18 12:11 ` Re: Allow specifying a dbname in pg_basebackup connection string Daniel Gustafsson <[email protected]>
2023-09-21 12:53 ` Re: Allow specifying a dbname in pg_basebackup connection string Daniel Gustafsson <[email protected]>
2024-11-04 17:14 ` Re: Allow specifying a dbname in pg_basebackup connection string Tom Lane <[email protected]>
2024-11-04 18:20 ` Re: Allow specifying a dbname in pg_basebackup connection string Jelte Fennema-Nio <[email protected]>
2024-11-04 18:34 ` Re: Allow specifying a dbname in pg_basebackup connection string Daniel Gustafsson <[email protected]>
2024-11-04 18:57 ` Re: Allow specifying a dbname in pg_basebackup connection string Tom Lane <[email protected]>
2024-11-04 19:09 ` Re: Allow specifying a dbname in pg_basebackup connection string Jelte Fennema-Nio <[email protected]>
2024-11-04 19:18 ` Re: Allow specifying a dbname in pg_basebackup connection string Tom Lane <[email protected]>
2023-08-16 19:08 [PATCH v6 1/1] Introduce macros for protocol characters. Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox