Thread
-
[PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-08-16T09:07:40Z
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. -
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-08-16T11:27:42Z
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: > 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. > -
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-16T14:55:51Z
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
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-08-19T10:57:17Z
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
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-08-22T10:30:13Z
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: > 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 >
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Michael Paquier <michael@paquier.xyz> — 2024-10-07T04:46:30Z
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 -
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-10-09T11:11:33Z
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. -
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Robert Haas <robertmhaas@gmail.com> — 2024-12-17T16:42:01Z
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
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-12-19T12:07:51Z
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.
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Nishant Sharma <nishant.sharma@enterprisedb.com> — 2024-12-19T12:49:33Z
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). >
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Robert Haas <robertmhaas@gmail.com> — 2024-12-19T13:29:56Z
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
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Robert Haas <robertmhaas@gmail.com> — 2025-02-27T19:10:21Z
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
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-27T21:08:12Z
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
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Robert Haas <robertmhaas@gmail.com> — 2025-02-27T21:12:19Z
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
-
Re: [PROPOSAL] : Disallow use of empty column name in (column_name '') in ALTER or CREATE of foreign table.
Robert Haas <robertmhaas@gmail.com> — 2025-03-26T18:15:19Z
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