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....@gmail.com> wrote: > On Fri, Aug 16, 2024 at 8:26 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Nishant Sharma <nishant.sha...@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 >
v2-0001-Disallow-empty-Foreign-Table-column-option-name-i.patch
Description: Binary data
v2-0002-Test-Cases-Changes.patch
Description: Binary data
