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

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

From

Nishant Sharma

Date:

16 August, 15:07:40

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.

From

Tom Lane

Date:

16 August, 20:55:51

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.

From

Ashutosh Bapat

Date:

19 August, 16:57:17

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.

From

Robert Haas

Date:

17 December, 22:42:01

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.

From

Nishant Sharma

Date:

19 December, 18:49:33

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.

From

Robert Haas

Date:

19 December, 19:29:56

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