[PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

Started by Nishant Sharmaover 1 year ago15 messages

nishant.sharma@enterprisedb.com

over 1 year ago

Hi,

--------------------------------------------------------------------------------------------------------------
Actual column names used while creation of foreign table are not allowed to
be an
empty string, but when we use column_name as an empty string in OPTIONS
during
CREATE or ALTER of foreign tables, it is allowed.

*EXAMPLES:-*
1) CREATE FOREIGN TABLE test_fdw(*"" *VARCHAR(15), id VARCHAR(5)) SERVER
localhost_fdw OPTIONS (schema_name 'public', table_name 'test');
ERROR: zero-length delimited identifier at or near """"
LINE 1: CREATE FOREIGN TABLE test_fdw("" VARCHAR(15), id VARCHAR(5))...

2) CREATE FOREIGN TABLE test_fdw(name VARCHAR(15) *OPTIONS* *(column_name
'')*, id VARCHAR(5)) SERVER localhost_fdw OPTIONS (schema_name 'public',
table_name 'test');
CREATE FOREIGN TABLE

--------------------------------------------------------------------------------------------------------------

Due to the above, when we try to simply select a remote table, the remote
query uses
the empty column name from the FDW column option and the select fails.

*EXAMPLES:-*
1) select * from test_fdw;
ERROR: zero-length delimited identifier at or near """"
CONTEXT: remote SQL command: SELECT "", id FROM public.test

2) explain verbose select * from test_fdw;
QUERY PLAN
--------------------------------------------------------------------------
Foreign Scan on public.test_fdw (cost=100.00..297.66 rows=853 width=72)
Output: name, id
Remote SQL: SELECT "", id FROM public.test
(3 rows)

--------------------------------------------------------------------------------------------------------------

We can fix this issue either during fetching of FDW column option names
while
building remote query or we do not allow at CREATE or ALTER of foreign
tables itself.
We think it would be better to disallow adding the column_name option as
empty in
CREATE or ALTER itself as we do not allow empty actual column names for a
foreign
table. Unless I missed to understand the purpose of allowing column_name as
empty.

*THE PROPOSED SOLUTION OUTPUT:-*
1) CREATE FOREIGN TABLE test_fdw(name VARCHAR(15) OPTIONS *(column_name '')*,
id VARCHAR(5)) SERVER localhost_fdw OPTIONS (schema_name 'public',
table_name 'test');
ERROR: column generic option name cannot be empty

2) CREATE FOREIGN TABLE test_fdw(name VARCHAR(15), id VARCHAR(5)) SERVER
localhost_fdw OPTIONS (schema_name 'public', table_name 'test');
CREATE FOREIGN TABLE

ALTER FOREIGN TABLE test_fdw ALTER COLUMN id OPTIONS *(column_name '')*;
ERROR: column generic option name cannot be empty

--------------------------------------------------------------------------------------------------------------

PFA, the fix and test cases patches attached. I ran "make check world" and
do
not see any failure related to patches. But, I do see an existing failure
t/001_pgbench_with_server.pl

Regards,
Nishant.

P.S
Thanks to Jeevan Chalke and Suraj Kharage for their inputs for the proposal.

Nishant Sharma

nishant.sharma@enterprisedb.com

over 1 year ago

In reply to: Nishant Sharma (#1)

2 attachment(s)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

Oops...
I forgot to attach the patch. Thanks to Amul Sul for pointing that out. :)

On Fri, Aug 16, 2024 at 2:37 PM Nishant Sharma <
nishant.sharma@enterprisedb.com> wrote:

Show quoted text

Hi,

--------------------------------------------------------------------------------------------------------------
Actual column names used while creation of foreign table are not allowed
to be an
empty string, but when we use column_name as an empty string in OPTIONS
during
CREATE or ALTER of foreign tables, it is allowed.

*EXAMPLES:-*
1) CREATE FOREIGN TABLE test_fdw(*"" *VARCHAR(15), id VARCHAR(5)) SERVER
localhost_fdw OPTIONS (schema_name 'public', table_name 'test');
ERROR: zero-length delimited identifier at or near """"
LINE 1: CREATE FOREIGN TABLE test_fdw("" VARCHAR(15), id VARCHAR(5))...

2) CREATE FOREIGN TABLE test_fdw(name VARCHAR(15) *OPTIONS* *(column_name
'')*, id VARCHAR(5)) SERVER localhost_fdw OPTIONS (schema_name 'public',
table_name 'test');
CREATE FOREIGN TABLE

postgres@43832=#\d test_fdw
Foreign table "public.test_fdw"
Column | Type | Collation | Nullable | Default | FDW
options

--------+-----------------------+-----------+----------+---------+------------------
name | character varying(15) | | | | *(column_name
'')*
id | character varying(5) | | | |
Server: localhost_fdw
FDW options: (schema_name 'public', table_name 'test')

--------------------------------------------------------------------------------------------------------------

Due to the above, when we try to simply select a remote table, the remote
query uses
the empty column name from the FDW column option and the select fails.

*EXAMPLES:-*
1) select * from test_fdw;
ERROR: zero-length delimited identifier at or near """"
CONTEXT: remote SQL command: SELECT "", id FROM public.test

2) explain verbose select * from test_fdw;
QUERY PLAN
--------------------------------------------------------------------------
Foreign Scan on public.test_fdw (cost=100.00..297.66 rows=853 width=72)
Output: name, id
Remote SQL: SELECT "", id FROM public.test
(3 rows)

--------------------------------------------------------------------------------------------------------------

We can fix this issue either during fetching of FDW column option names
while
building remote query or we do not allow at CREATE or ALTER of foreign
tables itself.
We think it would be better to disallow adding the column_name option as
empty in
CREATE or ALTER itself as we do not allow empty actual column names for a
foreign
table. Unless I missed to understand the purpose of allowing column_name
as empty.

*THE PROPOSED SOLUTION OUTPUT:-*
1) CREATE FOREIGN TABLE test_fdw(name VARCHAR(15) OPTIONS *(column_name
'')*, id VARCHAR(5)) SERVER localhost_fdw OPTIONS (schema_name 'public',
table_name 'test');
ERROR: column generic option name cannot be empty

2) CREATE FOREIGN TABLE test_fdw(name VARCHAR(15), id VARCHAR(5)) SERVER
localhost_fdw OPTIONS (schema_name 'public', table_name 'test');
CREATE FOREIGN TABLE

ALTER FOREIGN TABLE test_fdw ALTER COLUMN id OPTIONS *(column_name '')*;
ERROR: column generic option name cannot be empty

--------------------------------------------------------------------------------------------------------------

PFA, the fix and test cases patches attached. I ran "make check world" and
do
not see any failure related to patches. But, I do see an existing failure
t/001_pgbench_with_server.pl

Regards,
Nishant.

P.S
Thanks to Jeevan Chalke and Suraj Kharage for their inputs for the
proposal.

Attachments:

v1-0001-Disallow-empty-Foreign-Table-column-option-name-i.patchapplication/octet-stream; name=v1-0001-Disallow-empty-Foreign-Table-column-option-name-i.patchDownload

From ad4932955c855d056cb0df96a5d55931678f1819 Mon Sep 17 00:00:00 2001
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Wed, 14 Aug 2024 16:45:52 +0530
Subject: [PATCH v1 1/2] Disallow empty Foreign Table column option name i.e
 (column_name '').

---
 src/backend/commands/tablecmds.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0b2a524..ff8d6ba 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14214,10 +14214,26 @@ ATExecAlterColumnGenericOptions(Relation rel,
 	Form_pg_attribute atttableform;
 	AttrNumber	attnum;
 	ObjectAddress address;
+	ListCell   *lc;
 
 	if (options == NIL)
 		return InvalidObjectAddress;
 
+	foreach(lc, options)
+	{
+		DefElem    *def = (DefElem *) lfirst(lc);
+
+		if (strcmp(def->defname, "column_name") == 0)
+		{
+			char	   *gen_col_opt = defGetString(def);
+
+			if (gen_col_opt && gen_col_opt[0] == '\0')
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("column generic option name cannot be empty")));
+		}
+	}
+
 	/* First, determine FDW validator associated to the foreign table. */
 	ftrel = table_open(ForeignTableRelationId, AccessShareLock);
 	tuple = SearchSysCache1(FOREIGNTABLEREL, ObjectIdGetDatum(rel->rd_id));
-- 
1.8.3.1

v1-0002-Test-Cases-Changes.patchapplication/octet-stream; name=v1-0002-Test-Cases-Changes.patchDownload

From fe21257e9dc9869ea48f88ee7dd1ea4f0c8ebcb3 Mon Sep 17 00:00:00 2001
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Fri, 16 Aug 2024 10:14:58 +0530
Subject: [PATCH v1 2/2] Test Cases Changes

---
 contrib/postgres_fdw/expected/postgres_fdw.out | 8 ++++++++
 contrib/postgres_fdw/sql/postgres_fdw.sql      | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 2124347..bd150eb 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -139,6 +139,12 @@ CREATE FOREIGN TABLE ft7 (
 	c2 int NOT NULL,
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+ERROR:  column generic option name cannot be empty
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -200,6 +206,8 @@ ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
+ERROR:  column generic option name cannot be empty
 \det+
                               List of foreign tables
  Schema | Table |  Server   |              FDW options              | Description 
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 371e131..30c286d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -153,6 +153,12 @@ CREATE FOREIGN TABLE ft7 (
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
 
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -217,6 +223,7 @@ ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
 \det+
 
 -- Test that alteration of server options causes reconnection
-- 
1.8.3.1

Tom Lane

tgl@sss.pgh.pa.us

over 1 year ago

In reply to: Nishant Sharma (#1)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

Nishant Sharma <nishant.sharma@enterprisedb.com> writes:

Actual column names used while creation of foreign table are not allowed to
be an
empty string, but when we use column_name as an empty string in OPTIONS
during
CREATE or ALTER of foreign tables, it is allowed.

Is this really a bug? The valid remote names are determined by
whatever underlies the FDW, and I doubt we should assume that
SQL syntax restrictions apply to every FDW. Perhaps it would
be reasonable to apply such checks locally in SQL-based FDWs,
but I object to assuming such things at the level of
ATExecAlterColumnGenericOptions.

More generally, I don't see any meaningful difference between
this mistake and the more common one of misspelling the remote
column name, which is something we're not going to be able
to check for (at least not in anything like this way). If
you wanted to move the ease-of-use goalposts materially,
you should be looking for a way to do that.

regards, tom lane

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

over 1 year ago

In reply to: Tom Lane (#3)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Fri, Aug 16, 2024 at 8:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nishant Sharma <nishant.sharma@enterprisedb.com> writes:

Actual column names used while creation of foreign table are not allowed to
be an
empty string, but when we use column_name as an empty string in OPTIONS
during
CREATE or ALTER of foreign tables, it is allowed.

Is this really a bug? The valid remote names are determined by
whatever underlies the FDW, and I doubt we should assume that
SQL syntax restrictions apply to every FDW. Perhaps it would
be reasonable to apply such checks locally in SQL-based FDWs,
but I object to assuming such things at the level of
ATExecAlterColumnGenericOptions.

I agree.

More generally, I don't see any meaningful difference between
this mistake and the more common one of misspelling the remote
column name, which is something we're not going to be able
to check for (at least not in anything like this way). If
you wanted to move the ease-of-use goalposts materially,
you should be looking for a way to do that.

I think this check should be delegated to an FDW validator.

--
Best Wishes,
Ashutosh Bapat

Nishant Sharma

nishant.sharma@enterprisedb.com

over 1 year ago

In reply to: Ashutosh Bapat (#4)

2 attachment(s)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

Thanks Tom and Ashutosh for your responses!

I also agree that, v1 patch set was applying SQL syntax restrictions to all
FDWs,
which is not reasonable.

PFA v2 patch set. This is based on the suggestion given by Ashutosh to have
the
check in postgres_fdw validator. As it fits to apply the SQL syntax
restriction only
to FDWs which follow SQL syntax restrictions.
With this change, it gives hint/idea to any user using PostgresSQL with
their own
FDWs to add this check in their equivalent fdw validator.

Regarding, 'empty' vs 'misspelled' remote column name, from my point of
view,
I see some differences between them:-
1. SYNTAX wise - SQL syntax has restrictions for not allowing creating
column
names as empty. And it does not bother about, whether user used a misspelled
word or not for the column name.
2. IMPLEMENTATION wise - we don't need any extra info to decide that the
column
name received from command is empty, but we do need column name infos from
remote server to decide whether user misspelled the remote column name,
which
not only applies to the column_name options, but maybe to the column names
used
while creating the table syntax for foreign tables if the fdw column_name
option is
not added. Ex:- "CREATE FOREIGN TABLE test_fdw(name VARCHAR(15),
id VARCHAR(5)) ...." - to 'name' and 'id' here.

I may be wrong, but just wanted to share my thoughts on the differences.
So, it
can be considered a different issue/mistake and can be handled separately in
another email thread.

I ran "make check world" and do not see any failure related to patches.
But, I do see
an existing failure "t/001_pgbench_with_server.pl".

Regards,
Nishant Sharma
EnterpriseDB, Pune.

On Mon, Aug 19, 2024 at 4:27 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

Show quoted text

On Fri, Aug 16, 2024 at 8:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nishant Sharma <nishant.sharma@enterprisedb.com> writes:

Actual column names used while creation of foreign table are not

allowed to

be an
empty string, but when we use column_name as an empty string in OPTIONS
during
CREATE or ALTER of foreign tables, it is allowed.

Is this really a bug? The valid remote names are determined by
whatever underlies the FDW, and I doubt we should assume that
SQL syntax restrictions apply to every FDW. Perhaps it would
be reasonable to apply such checks locally in SQL-based FDWs,
but I object to assuming such things at the level of
ATExecAlterColumnGenericOptions.

I agree.

More generally, I don't see any meaningful difference between
this mistake and the more common one of misspelling the remote
column name, which is something we're not going to be able
to check for (at least not in anything like this way). If
you wanted to move the ease-of-use goalposts materially,
you should be looking for a way to do that.

I think this check should be delegated to an FDW validator.

--
Best Wishes,
Ashutosh Bapat

Attachments:

v2-0001-Disallow-empty-Foreign-Table-column-option-name-i.patchapplication/octet-stream; name=v2-0001-Disallow-empty-Foreign-Table-column-option-name-i.patchDownload

From 16d2cb15ae67beb646ba931993bff15dab356f00 Mon Sep 17 00:00:00 2001
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Thu, 22 Aug 2024 12:53:43 +0530
Subject: [PATCH v2 1/2] Disallow empty Foreign Table column option name i.e
 (column_name '') for postgres_fdw.

---
 contrib/postgres_fdw/option.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 630b304..2f76610 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -228,6 +228,19 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 						 errmsg("invalid value for string option \"%s\": %s",
 								def->defname, value)));
 		}
+		else if (strcmp(def->defname, "column_name") == 0)
+		{
+			char	   *col_name_opt = defGetString(def);
+
+			/*
+			 * PostgresSQL follows SQL syntax, so we do not allow empty
+			 * column_name option.
+			 */
+			if (col_name_opt && col_name_opt[0] == '\0')
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("colum_name option cannot be empty for postgres_fdw")));
+		}
 	}
 
 	PG_RETURN_VOID();
-- 
1.8.3.1

v2-0002-Test-Cases-Changes.patchapplication/octet-stream; name=v2-0002-Test-Cases-Changes.patchDownload

From 79e409d4914727c9974b9cd0a48ebf12d36e8f21 Mon Sep 17 00:00:00 2001
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Thu, 22 Aug 2024 13:11:15 +0530
Subject: [PATCH v2 2/2] Test Cases Changes

---
 contrib/postgres_fdw/expected/postgres_fdw.out | 8 ++++++++
 contrib/postgres_fdw/sql/postgres_fdw.sql      | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index f3eb055..b6d0a61 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -139,6 +139,12 @@ CREATE FOREIGN TABLE ft7 (
 	c2 int NOT NULL,
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+ERROR:  colum_name option cannot be empty for postgres_fdw
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -200,6 +206,8 @@ ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
+ERROR:  colum_name option cannot be empty for postgres_fdw
 \det+
                               List of foreign tables
  Schema | Table |  Server   |              FDW options              | Description 
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 0734716..7a1c590 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -153,6 +153,12 @@ CREATE FOREIGN TABLE ft7 (
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
 
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -217,6 +223,7 @@ ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
 \det+
 
 -- Test that alteration of server options causes reconnection
-- 
1.8.3.1

Michael Paquier

michael@paquier.xyz

over 1 year ago

In reply to: Nishant Sharma (#5)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Thu, Aug 22, 2024 at 04:00:13PM +0530, Nishant Sharma wrote:

I may be wrong, but just wanted to share my thoughts on the differences.
So, it
can be considered a different issue/mistake and can be handled separately in
another email thread.

+        else if (strcmp(def->defname, "column_name") == 0)
+        {
+            char       *col_name_opt = defGetString(def);
+
+            /*
+             * PostgresSQL follows SQL syntax, so we do not allow empty
+             * column_name option.
+             */
+            if (col_name_opt && col_name_opt[0] == '\0')
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                         errmsg("colum_name option cannot be empty for postgres_fdw")));
+        }

If we begin to care about empty names in column_name in the FDW
command, shouldn't we also care about empry values in schema_name and
table_name?

Typos: PostgresSQL -> PostgreSQL and colum_name -> column_name.

Once you associate table_name and schema_name, you can save in
translation by rewording the errmsg like that (no need to mention
postgres_fdw, note the quotes around the option name):
errmsg("cannot use empty value for option \"%s\"",
"column_name");
--
Michael

Nishant Sharma

nishant.sharma@enterprisedb.com

over 1 year ago

In reply to: Michael Paquier (#6)

2 attachment(s)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Mon, Oct 7, 2024 at 10:16 AM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Aug 22, 2024 at 04:00:13PM +0530, Nishant Sharma wrote:

I may be wrong, but just wanted to share my thoughts on the differences.
So, it
can be considered a different issue/mistake and can be handled

separately in

another email thread.
+        else if (strcmp(def->defname, "column_name") == 0)
+        {
+            char       *col_name_opt = defGetString(def);
+
+            /*
+             * PostgresSQL follows SQL syntax, so we do not allow empty
+             * column_name option.
+             */
+            if (col_name_opt && col_name_opt[0] == '\0')
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                         errmsg("colum_name option cannot be empty for
postgres_fdw")));
+        }
If we begin to care about empty names in column_name in the FDW
command, shouldn't we also care about empry values in schema_name and
table_name?

Typos: PostgresSQL -> PostgreSQL and colum_name -> column_name.

Once you associate table_name and schema_name, you can save in
translation by rewording the errmsg like that (no need to mention
postgres_fdw, note the quotes around the option name):
errmsg("cannot use empty value for option \"%s\"",
"column_name");
--
Michael

Thanks Michael for your review comments and response!

I have included them in v3. v3 does not allow empty schema_name &
table_name options along with column_name.

Thanks,
Nishant.

Attachments:

v3-0002-Test-Cases-Changes.patchapplication/octet-stream; name=v3-0002-Test-Cases-Changes.patchDownload

From 187546e84e185bea6652507eb230b592e0f94b2e Mon Sep 17 00:00:00 2001
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Wed, 9 Oct 2024 15:45:58 +0530
Subject: [PATCH v3 2/2] Test Cases Changes

---
 contrib/postgres_fdw/expected/postgres_fdw.out | 24 ++++++++++++++++++++++++
 contrib/postgres_fdw/sql/postgres_fdw.sql      | 21 +++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index f2bcd6a..d1e3bd9 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -139,6 +139,24 @@ CREATE FOREIGN TABLE ft7 (
 	c2 int NOT NULL,
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+ERROR:  cannot use empty value for option "column_name"
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name '', table_name 'T 4');
+ERROR:  cannot use empty value for option "schema_name"
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name '');
+ERROR:  cannot use empty value for option "table_name"
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -196,10 +214,16 @@ ALTER USER MAPPING FOR public SERVER testserver1
 -- permitted to check validation.
 ALTER USER MAPPING FOR public SERVER testserver1
 	OPTIONS (ADD sslkey 'value', ADD sslcert 'value');
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name '', table_name 'T 1');
+ERROR:  cannot use empty value for option "schema_name"
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name '');
+ERROR:  cannot use empty value for option "table_name"
 ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
+ERROR:  cannot use empty value for option "column_name"
 \det+
                               List of foreign tables
  Schema | Table |  Server   |              FDW options              | Description 
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 372fe6d..c8073b2 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -153,6 +153,24 @@ CREATE FOREIGN TABLE ft7 (
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
 
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name '', table_name 'T 4');
+
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name '');
+
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -213,10 +231,13 @@ ALTER USER MAPPING FOR public SERVER testserver1
 ALTER USER MAPPING FOR public SERVER testserver1
 	OPTIONS (ADD sslkey 'value', ADD sslcert 'value');
 
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name '', table_name 'T 1');
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name '');
 ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
 \det+
 
 -- Test that alteration of server options causes reconnection
-- 
1.8.3.1

v3-0001-Disallow-empty-Foreign-Table-column-option-name-i.patchapplication/octet-stream; name=v3-0001-Disallow-empty-Foreign-Table-column-option-name-i.patchDownload

From 3693492f38743b1431484e6845fd5b668299b501 Mon Sep 17 00:00:00 2001
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Wed, 9 Oct 2024 14:55:59 +0530
Subject: [PATCH v3 1/2] Disallow empty Foreign Table column option name i.e 
 (column_name '') for postgres_fdw.

---
 contrib/postgres_fdw/option.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index d740893..dfc95ea 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -228,6 +228,22 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 						 errmsg("invalid value for string option \"%s\": %s",
 								def->defname, value)));
 		}
+		else if (strcmp(def->defname, "column_name") == 0 ||
+				 strcmp(def->defname, "schema_name") == 0 ||
+				 strcmp(def->defname, "table_name") == 0)
+		{
+			char	   *obj_name_opt = defGetString(def);
+
+			/*
+			 * PostgreSQL follows SQL syntax, so we do not allow empty
+			 * column_name, schema_name & table_name options.
+			 */
+			if (obj_name_opt && obj_name_opt[0] == '\0')
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("cannot use empty value for option \"%s\"",
+								def->defname)));
+		}
 	}
 
 	PG_RETURN_VOID();
-- 
1.8.3.1

Robert Haas

robertmhaas@gmail.com

about 1 year ago

In reply to: Nishant Sharma (#7)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Wed, Oct 9, 2024 at 7:12 AM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:

I have included them in v3. v3 does not allow empty schema_name &
table_name options along with column_name.

I was looking at these patches today and trying to understand whether
the proposed error message is consistent with what we have done
elsewhere.

The proposed error message was "cannot use empty value for option
\"%s\". I looked for error messages that contained the phrase "empty"
or "zero". I did not find a case exactly like this one. The closet
analogues I found were things like this:

invalid cursor name: must not be empty
output cannot be empty string
DSM segment name cannot be empty
row path filter must not be empty string

These messages aren't quite as consistent as one might wish, so it's a
little hard to judge here. Nonetheless, I propose that the error
message we use here should end with either "cannot" or "must not"
followed by either "be empty" or "be empty string". I think my
preferred variant would be "value for option \"%s\" must not be empty
string". But there's certainly oodles of room for bikeshedding.

Apart from that, I see very little to complain about here. Once we
agree on the error message text I think something can be committed.
For everyone's convenience, I suggest merging the two patches into
one. I highly approve of separating patches by topic, but there's
usually no point in separating them by directory.

--
Robert Haas
EDB: http://www.enterprisedb.com

Nishant Sharma

nishant.sharma@enterprisedb.com

about 1 year ago

In reply to: Robert Haas (#8)

1 attachment(s)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Tue, Dec 17, 2024 at 10:12 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 9, 2024 at 7:12 AM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:

I have included them in v3. v3 does not allow empty schema_name &
table_name options along with column_name.

I was looking at these patches today and trying to understand whether
the proposed error message is consistent with what we have done
elsewhere.

The proposed error message was "cannot use empty value for option
\"%s\". I looked for error messages that contained the phrase "empty"
or "zero". I did not find a case exactly like this one. The closet
analogues I found were things like this:

invalid cursor name: must not be empty
output cannot be empty string
DSM segment name cannot be empty
row path filter must not be empty string

These messages aren't quite as consistent as one might wish, so it's a
little hard to judge here. Nonetheless, I propose that the error
message we use here should end with either "cannot" or "must not"
followed by either "be empty" or "be empty string". I think my
preferred variant would be "value for option \"%s\" must not be empty
string". But there's certainly oodles of room for bikeshedding.

Apart from that, I see very little to complain about here. Once we
agree on the error message text I think something can be committed.
For everyone's convenience, I suggest merging the two patches into
one. I highly approve of separating patches by topic, but there's
usually no point in separating them by directory.

--
Robert Haas
EDB: http://www.enterprisedb.com

Thanks Robert for your response and the review comments!

I have addressed both your suggestions, by changing the error
message and merging both the patches to one.

PFA, patch set v4.

Regards,
Nishant.

Attachments:

v4-0001-Disallow-empty-Foreign-Table-column_name-schema_n.patchapplication/octet-stream; name=v4-0001-Disallow-empty-Foreign-Table-column_name-schema_n.patchDownload

From 1f2a4d378dc7f540d05f12a20e84547132ae2e02 Mon Sep 17 00:00:00 2001
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Thu, 19 Dec 2024 16:11:07 +0530
Subject: [PATCH v4] Disallow empty Foreign Table column_name, schema_name,
 table_name options for postgres_fdw.

---
 contrib/postgres_fdw/expected/postgres_fdw.out | 24 ++++++++++++++++++++++++
 contrib/postgres_fdw/option.c                  | 16 ++++++++++++++++
 contrib/postgres_fdw/sql/postgres_fdw.sql      | 21 +++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index bf32219..ef5d93e 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -139,6 +139,24 @@ CREATE FOREIGN TABLE ft7 (
 	c2 int NOT NULL,
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+ERROR:  value for option "column_name" must not be empty string
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name '', table_name 'T 4');
+ERROR:  value for option "schema_name" must not be empty string
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name '');
+ERROR:  value for option "table_name" must not be empty string
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -196,10 +214,16 @@ ALTER USER MAPPING FOR public SERVER testserver1
 -- permitted to check validation.
 ALTER USER MAPPING FOR public SERVER testserver1
 	OPTIONS (ADD sslkey 'value', ADD sslcert 'value');
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name '', table_name 'T 1');
+ERROR:  value for option "schema_name" must not be empty string
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name '');
+ERROR:  value for option "table_name" must not be empty string
 ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
+ERROR:  value for option "column_name" must not be empty string
 \det+
                               List of foreign tables
  Schema | Table |  Server   |              FDW options              | Description 
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 232d853..63bbd88 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -227,6 +227,22 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 						 errmsg("invalid value for string option \"%s\": %s",
 								def->defname, value)));
 		}
+		else if (strcmp(def->defname, "column_name") == 0 ||
+				 strcmp(def->defname, "schema_name") == 0 ||
+				 strcmp(def->defname, "table_name") == 0)
+		{
+			char	   *obj_name_opt = defGetString(def);
+
+			/*
+			 * PostgreSQL follows SQL syntax, so we do not allow empty
+			 * column_name, schema_name & table_name options.
+			 */
+			if (obj_name_opt && obj_name_opt[0] == '\0')
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("value for option \"%s\" must not be empty string",
+								def->defname)));
+		}
 	}
 
 	PG_RETURN_VOID();
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 3900522..477a58a 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -153,6 +153,24 @@ CREATE FOREIGN TABLE ft7 (
 	c3 text
 ) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
 
+CREATE FOREIGN TABLE ft8 (
+	c1 int OPTIONS (column_name '') NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
+
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name '', table_name 'T 4');
+
+CREATE FOREIGN TABLE ft8 (
+	c1 int NOT NULL,
+	c2 int NOT NULL,
+	c3 text
+) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name '');
+
 -- ===================================================================
 -- tests for validator
 -- ===================================================================
@@ -213,10 +231,13 @@ ALTER USER MAPPING FOR public SERVER testserver1
 ALTER USER MAPPING FOR public SERVER testserver1
 	OPTIONS (ADD sslkey 'value', ADD sslcert 'value');
 
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name '', table_name 'T 1');
+ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name '');
 ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
 ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
+ALTER FOREIGN TABLE ft2 ALTER COLUMN c2 OPTIONS (column_name '');
 \det+
 
 -- Test that alteration of server options causes reconnection
-- 
1.8.3.1

#10

Nishant Sharma

nishant.sharma@enterprisedb.com

about 1 year ago

In reply to: Nishant Sharma (#9)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

Hi,

Summary of this thread-

Tom had the following concern:-
Regarding location of check in
ATExecAlterColumnGenericOptions. And spell check issue
comparison. Which I believe was addressed by v2 and
its response.

Ashutosh had the suggestion:-
Check should be delegated to an FDW validator. Which I
believe was addressed in v2.

Michael had the following concern:-
We should also care about empty values in schema_name
and table_name. Which I believe I have addressed in v3
patch.

Robert had the following concern:-
Regarding error message and single patch for this. Which I
believe I have addressed in v4 patch.

Tom, Ashutosh, Michael, Robert please let me know if I was
able to address your concerns or if you feel differently.

Assuming Tom, Ashutosh, Michael and Robert feel as though
I have addressed the concerns mentioned above, does anybody
have any additional feedback or concerns with this patch? If not,
I would request we move to commit phase.
Thank you!

Regards,
Nishant Sharma (EDB).

Show quoted text

#11

Robert Haas

robertmhaas@gmail.com

about 1 year ago

In reply to: Nishant Sharma (#9)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Thu, Dec 19, 2024 at 7:08 AM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:

Thanks Robert for your response and the review comments!

I have addressed both your suggestions, by changing the error
message and merging both the patches to one.

PFA, patch set v4.

Cool. I don't want to commit this right before my vacation but I'll
look into it in the new year if nobody beats me to it.

--
Robert Haas
EDB: http://www.enterprisedb.com

#12

Robert Haas

robertmhaas@gmail.com

11 months ago

In reply to: Robert Haas (#11)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Thu, Dec 19, 2024 at 8:29 AM Robert Haas <robertmhaas@gmail.com> wrote:

Cool. I don't want to commit this right before my vacation but I'll
look into it in the new year if nobody beats me to it.

Nobody has beaten me to it, but I thought of one potential issue.
Suppose that somebody has a foreign table that exists today with one
of these properties set to the empty string. Such a foreign table is
not usable for queries, because the queries won't make it past scan.l
on the remote side, but it's allowed to exist. With this patch, it's
not allowed to exist any more. That would be fine if no such objects
exist in the wild, but if they do, people will get a pg_dump or
pg_upgrade failure when they try to upgrade to a release that includes
this change. Does that mean that we shouldn't commit this patch?

I'm inclined to think that it's probably still OK, because (1) few
users are likely to have such objects, (2) the workaround is easy,
just drop the object or fix it to do something useful, just as users
already need to do in plenty of other, similar cases and (3) unclear
error messages are not great and future users might benefit from this
error message being better. However, one could take the opposite
position and say that (C1) it is unlikely that anyone will try to do
this in the first place and (C2) the error message that is currently
generated when you try to do this isn't really all that unclear, and
therefore it's not worth creating even a minor dump-and-reload hazard;
and that position also seems pretty defensible to me.

Other opinions?

--
Robert Haas
EDB: http://www.enterprisedb.com

#13

Tom Lane

tgl@sss.pgh.pa.us

11 months ago

In reply to: Robert Haas (#12)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

Robert Haas <robertmhaas@gmail.com> writes:

Nobody has beaten me to it, but I thought of one potential issue.
Suppose that somebody has a foreign table that exists today with one
of these properties set to the empty string. Such a foreign table is
not usable for queries, because the queries won't make it past scan.l
on the remote side, but it's allowed to exist.

Hmm. A query not using that column would work just fine, so it's
at least arguable that there are such beasts in reality and the
user hasn't noticed the bug. (I doubt this, but if we're worrying
about improbable failures...)

With this patch, it's
not allowed to exist any more. That would be fine if no such objects
exist in the wild, but if they do, people will get a pg_dump or
pg_upgrade failure when they try to upgrade to a release that includes
this change. Does that mean that we shouldn't commit this patch?

I was down on it to start with, on the grounds that it wasn't adding
enough ease-of-use to justify even a relatively small amount of added
code. I'm not sure that the dump-reload failure angle is much of a
concern in reality, but I would have voted not to commit before and
I still do.

regards, tom lane

#14

Robert Haas

robertmhaas@gmail.com

11 months ago

In reply to: Tom Lane (#13)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Thu, Feb 27, 2025 at 4:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I was down on it to start with, on the grounds that it wasn't adding
enough ease-of-use to justify even a relatively small amount of added
code. I'm not sure that the dump-reload failure angle is much of a
concern in reality, but I would have voted not to commit before and
I still do.

OK, fair. Anyone want to argue the other side?

--
Robert Haas
EDB: http://www.enterprisedb.com

#15

Robert Haas

robertmhaas@gmail.com

10 months ago

In reply to: Robert Haas (#14)

Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.

On Thu, Feb 27, 2025 at 4:12 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Feb 27, 2025 at 4:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I was down on it to start with, on the grounds that it wasn't adding
enough ease-of-use to justify even a relatively small amount of added
code. I'm not sure that the dump-reload failure angle is much of a
concern in reality, but I would have voted not to commit before and
I still do.

OK, fair. Anyone want to argue the other side?

Apparently not, hence I am marking this Rejected.

--
Robert Haas
EDB: http://www.enterprisedb.com