public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v3] Cleanup/remove/update references to OID column... 47+ messages / 15 participants [nested] [flat]
* [PATCH v3] Cleanup/remove/update references to OID column... @ 2019-04-03 00:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Justin Pryzby @ 2019-04-03 00:13 UTC (permalink / raw) ..in wake of 578b229718e8f. See also 93507e67c9ca54026019ebec3026de35d30370f9 1464755fc490a9911214817fe83077a3689250ab Author: Justin Pryzby <[email protected]> Author: Daniel Verite <[email protected]> --- doc/src/sgml/information_schema.sgml | 4 ++-- doc/src/sgml/ref/insert.sgml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml index 234a3bb..9c618b1 100644 --- a/doc/src/sgml/information_schema.sgml +++ b/doc/src/sgml/information_schema.sgml @@ -1312,8 +1312,8 @@ <para> The view <literal>columns</literal> contains information about all table columns (or view columns) in the database. System columns - (<literal>ctid</literal>, etc.) are not included. Only those columns are - shown that the current user has access to (by way of being the + (<literal>ctid</literal>, etc.) are not included. The only columns shown + are those to which the current user has access (by way of being the owner or having some privilege). </para> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml index 189ce2a..f995a76 100644 --- a/doc/src/sgml/ref/insert.sgml +++ b/doc/src/sgml/ref/insert.sgml @@ -554,7 +554,7 @@ INSERT <replaceable>oid</replaceable> <replaceable class="parameter">count</repl The <replaceable class="parameter">count</replaceable> is the number of rows inserted or updated. <replaceable>oid</replaceable> is always 0 (it used to be the <acronym>OID</acronym> assigned to the inserted row if - <replaceable>rows</replaceable> was exactly one and the target table was + <replaceable>count</replaceable> was exactly one and the target table was declared <literal>WITH OIDS</literal> and 0 otherwise, but creating a table <literal>WITH OIDS</literal> is not supported anymore). </para> -- 2.7.4 --TKYYegg/GYAC5JIZ-- ^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH v2] Cleanup/remove/update references to OID column... @ 2019-04-03 00:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Justin Pryzby @ 2019-04-03 00:13 UTC (permalink / raw) ..in wake of 578b229718e8f. See also 93507e67c9ca54026019ebec3026de35d30370f9 1464755fc490a9911214817fe83077a3689250ab Reviewed-by: Daniel Verite <[email protected]> --- doc/src/sgml/catalogs.sgml | 2 +- doc/src/sgml/ddl.sgml | 12 +++++------- doc/src/sgml/information_schema.sgml | 4 ++-- doc/src/sgml/ref/create_trigger.sgml | 2 +- doc/src/sgml/ref/insert.sgml | 12 +++++------- doc/src/sgml/ref/psql-ref.sgml | 3 +++ doc/src/sgml/spi.sgml | 2 +- 7 files changed, 18 insertions(+), 19 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 40ddec4..d544e60 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -1047,7 +1047,7 @@ <entry></entry> <entry> The number of the column. Ordinary columns are numbered from 1 - up. System columns, such as <structfield>oid</structfield>, + up. System columns, such as <structfield>ctid</structfield>, have (arbitrary) negative numbers. </entry> </row> diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 9e761db..244d5ce 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -1202,8 +1202,7 @@ CREATE TABLE circles ( <structfield>ctid</structfield> will change if it is updated or moved by <command>VACUUM FULL</command>. Therefore <structfield>ctid</structfield> is useless as a long-term row - identifier. The OID, or even better a user-defined serial - number, should be used to identify logical rows. + identifier. A primary key should be used to identify logical rows. </para> </listitem> </varlistentry> @@ -3672,11 +3671,10 @@ VALUES ('Albany', NULL, NULL, 'NY'); <para> Partitions cannot have columns that are not present in the parent. It is not possible to specify columns when creating partitions with - <command>CREATE TABLE</command>, nor is it possible to add columns to - partitions after-the-fact using <command>ALTER TABLE</command>. Tables may be - added as a partition with <command>ALTER TABLE ... ATTACH PARTITION</command> - only if their columns exactly match the parent, including any - <literal>oid</literal> column. + <command>CREATE TABLE</command>, to add columns to + partitions after-the-fact using <command>ALTER TABLE</command>, nor to + add a partition with <command>ALTER TABLE ... ATTACH PARTITION</command> + if its columns would not exactly match those of the parent. </para> </listitem> diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml index 1321ade..9c618b1 100644 --- a/doc/src/sgml/information_schema.sgml +++ b/doc/src/sgml/information_schema.sgml @@ -1312,8 +1312,8 @@ <para> The view <literal>columns</literal> contains information about all table columns (or view columns) in the database. System columns - (<literal>oid</literal>, etc.) are not included. Only those columns are - shown that the current user has access to (by way of being the + (<literal>ctid</literal>, etc.) are not included. The only columns shown + are those to which the current user has access (by way of being the owner or having some privilege). </para> diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml index 6456105..3339a4b 100644 --- a/doc/src/sgml/ref/create_trigger.sgml +++ b/doc/src/sgml/ref/create_trigger.sgml @@ -465,7 +465,7 @@ UPDATE OF <replaceable>column_name1</replaceable> [, <replaceable>column_name2</ that the <literal>NEW</literal> row seen by the condition is the current value, as possibly modified by earlier triggers. Also, a <literal>BEFORE</literal> trigger's <literal>WHEN</literal> condition is not allowed to examine the - system columns of the <literal>NEW</literal> row (such as <literal>oid</literal>), + system columns of the <literal>NEW</literal> row (such as <literal>ctid</literal>), because those won't have been set yet. </para> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml index 62e142f..3e1be4c 100644 --- a/doc/src/sgml/ref/insert.sgml +++ b/doc/src/sgml/ref/insert.sgml @@ -552,13 +552,11 @@ INSERT INTO <replaceable class="parameter">table_name</replaceable> [ AS <replac INSERT <replaceable>oid</replaceable> <replaceable class="parameter">count</replaceable> </screen> The <replaceable class="parameter">count</replaceable> is the - number of rows inserted or updated. If <replaceable - class="parameter">count</replaceable> is exactly one, and the - target table has OIDs, then <replaceable - class="parameter">oid</replaceable> is the <acronym>OID</acronym> - assigned to the inserted row. The single row must have been - inserted rather than updated. Otherwise <replaceable - class="parameter">oid</replaceable> is zero. + number of rows inserted or updated. + <replaceable>oid</replaceable> used to be the object ID of the inserted row + if <replaceable>rows</replaceable> was 1 and the target table had OIDs, but + OIDs system columns are not supported anymore; therefore + <replaceable>oid</replaceable> is always 0. </para> <para> diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 08f4bab..0e6e792 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3794,6 +3794,9 @@ bar command. This variable is only guaranteed to be valid until after the result of the next <acronym>SQL</acronym> command has been displayed. + <productname>PostgreSQL</productname> servers since version 12 do not + support OID system columns in user tables, and LASTOID will always be 0 + following <command>INSERT</command>. </para> </listitem> </varlistentry> diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index 9b2f516..66eced6 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -3105,7 +3105,7 @@ int SPI_fnumber(TupleDesc <parameter>rowdesc</parameter>, const char * <paramete <para> If <parameter>colname</parameter> refers to a system column (e.g., - <literal>oid</literal>) then the appropriate negative column number will + <literal>ctid</literal>) then the appropriate negative column number will be returned. The caller should be careful to test the return value for exact equality to <symbol>SPI_ERROR_NOATTRIBUTE</symbol> to detect an error; testing the result for less than or equal to 0 is -- 2.1.4 --dFWYt1i2NyOo1oI9-- ^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH v1] Cleanup/remove/update references to OID column... @ 2019-04-03 00:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Justin Pryzby @ 2019-04-03 00:13 UTC (permalink / raw) To: [email protected] ..in wake of 578b229718e8f. See also 93507e67c9ca54026019ebec3026de35d30370f9 1464755fc490a9911214817fe83077a3689250ab --- doc/src/sgml/ddl.sgml | 9 ++++----- doc/src/sgml/ref/insert.sgml | 12 +++++------- doc/src/sgml/ref/psql-ref.sgml | 3 +++ 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 9e761db..db044c5 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -3672,11 +3672,10 @@ VALUES ('Albany', NULL, NULL, 'NY'); <para> Partitions cannot have columns that are not present in the parent. It is not possible to specify columns when creating partitions with - <command>CREATE TABLE</command>, nor is it possible to add columns to - partitions after-the-fact using <command>ALTER TABLE</command>. Tables may be - added as a partition with <command>ALTER TABLE ... ATTACH PARTITION</command> - only if their columns exactly match the parent, including any - <literal>oid</literal> column. + <command>CREATE TABLE</command>, to add columns to + partitions after-the-fact using <command>ALTER TABLE</command>, nor to + add a partition with <command>ALTER TABLE ... ATTACH PARTITION</command> + if its columns would not exactly match those of the parent. </para> </listitem> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml index 62e142f..3e1be4c 100644 --- a/doc/src/sgml/ref/insert.sgml +++ b/doc/src/sgml/ref/insert.sgml @@ -552,13 +552,11 @@ INSERT INTO <replaceable class="parameter">table_name</replaceable> [ AS <replac INSERT <replaceable>oid</replaceable> <replaceable class="parameter">count</replaceable> </screen> The <replaceable class="parameter">count</replaceable> is the - number of rows inserted or updated. If <replaceable - class="parameter">count</replaceable> is exactly one, and the - target table has OIDs, then <replaceable - class="parameter">oid</replaceable> is the <acronym>OID</acronym> - assigned to the inserted row. The single row must have been - inserted rather than updated. Otherwise <replaceable - class="parameter">oid</replaceable> is zero. + number of rows inserted or updated. + <replaceable>oid</replaceable> used to be the object ID of the inserted row + if <replaceable>rows</replaceable> was 1 and the target table had OIDs, but + OIDs system columns are not supported anymore; therefore + <replaceable>oid</replaceable> is always 0. </para> <para> diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 08f4bab..0e6e792 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3794,6 +3794,9 @@ bar command. This variable is only guaranteed to be valid until after the result of the next <acronym>SQL</acronym> command has been displayed. + <productname>PostgreSQL</productname> servers since version 12 do not + support OID system columns in user tables, and LASTOID will always be 0 + following <command>INSERT</command>. </para> </listitem> </varlistentry> -- 2.1.4 --fUYQa+Pmc3FrFX/N-- ^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH v3 12/12] Cleanup/remove/update references to OID column... @ 2019-04-03 00:13 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Justin Pryzby @ 2019-04-03 00:13 UTC (permalink / raw) ..in wake of 578b229718e8f. See also 93507e67c9ca54026019ebec3026de35d30370f9 1464755fc490a9911214817fe83077a3689250ab f6b39171f3d65155b9390c2c69bc5b3469f923a8 --- doc/src/sgml/catalogs.sgml | 2 +- doc/src/sgml/information_schema.sgml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index d544e60..0f9c6f2 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -610,7 +610,7 @@ <entry><structfield>oid</structfield></entry> <entry><type>oid</type></entry> <entry></entry> - <entry>Row identifier (hidden attribute; must be explicitly selected)</entry> + <entry>Row identifier</entry> </row> <row> diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml index 234a3bb..9c618b1 100644 --- a/doc/src/sgml/information_schema.sgml +++ b/doc/src/sgml/information_schema.sgml @@ -1312,8 +1312,8 @@ <para> The view <literal>columns</literal> contains information about all table columns (or view columns) in the database. System columns - (<literal>ctid</literal>, etc.) are not included. Only those columns are - shown that the current user has access to (by way of being the + (<literal>ctid</literal>, etc.) are not included. The only columns shown + are those to which the current user has access (by way of being the owner or having some privilege). </para> -- 2.7.4 --cWoXeonUoKmBZSoM-- ^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH v1] Cleanup/remove/update references to OID column... @ 2019-05-08 18:57 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Justin Pryzby @ 2019-05-08 18:57 UTC (permalink / raw) ..in wake of 578b229718e8f. See also 93507e67c9ca54026019ebec3026de35d30370f9 1464755fc490a9911214817fe83077a3689250ab f6b39171f3d65155b9390c2c69bc5b3469f923a8 Author: Justin Pryzby <[email protected]> --- doc/src/sgml/catalogs.sgml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index d544e60..0f9c6f2 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -610,7 +610,7 @@ <entry><structfield>oid</structfield></entry> <entry><type>oid</type></entry> <entry></entry> - <entry>Row identifier (hidden attribute; must be explicitly selected)</entry> + <entry>Row identifier</entry> </row> <row> -- 2.7.4 --eDB11BtaWSyaBkpc-- ^ permalink raw reply [nested|flat] 47+ messages in thread
* drop postmaster symlink @ 2022-11-23 07:52 Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Peter Eisentraut @ 2022-11-23 07:52 UTC (permalink / raw) To: pgsql-hackers A little while ago we discussed briefly over in the meson thread whether we could remove the postmaster symlink [0]. The meson build system currently does not install a postmaster symlink. (AFAICT, the MSVC build system does not either.) So if we want to elevate the meson build system, we either need to add the postmaster symlink, or remove it from the other build system(s) as well. Seeing that it's been deprecated for a long time, I propose we just remove it. See attached patches. [0]: https://www.postgresql.org/message-id/[email protected] From 179eaab96dc5215970b2b3485cf77f1a58f9d337 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 13 Nov 2022 20:52:00 +0100 Subject: [PATCH 1/2] Remove gratuitous references to postmaster instead of postgres --- contrib/start-scripts/freebsd | 2 +- contrib/start-scripts/linux | 2 +- src/port/path.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/start-scripts/freebsd b/contrib/start-scripts/freebsd index 3323237a54b4..5e22b2d2f0e4 100644 --- a/contrib/start-scripts/freebsd +++ b/contrib/start-scripts/freebsd @@ -29,7 +29,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin # What to use to start up the postmaster. (If you want the script to wait # until the server has started, you could use "pg_ctl start" here.) -DAEMON="$prefix/bin/postmaster" +DAEMON="$prefix/bin/postgres" # What to use to shut down the postmaster PGCTL="$prefix/bin/pg_ctl" diff --git a/contrib/start-scripts/linux b/contrib/start-scripts/linux index a7757162fc4b..9292855df716 100644 --- a/contrib/start-scripts/linux +++ b/contrib/start-scripts/linux @@ -61,7 +61,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin # What to use to start up the postmaster. (If you want the script to wait # until the server has started, you could use "pg_ctl start" here.) -DAEMON="$prefix/bin/postmaster" +DAEMON="$prefix/bin/postgres" # What to use to shut down the postmaster PGCTL="$prefix/bin/pg_ctl" diff --git a/src/port/path.c b/src/port/path.c index 05fe812f757b..fb64873c7a43 100644 --- a/src/port/path.c +++ b/src/port/path.c @@ -651,7 +651,7 @@ dir_strcmp(const char *s1, const char *s2) * For example: * target_path = '/usr/local/share/postgresql' * bin_path = '/usr/local/bin' - * my_exec_path = '/opt/pgsql/bin/postmaster' + * my_exec_path = '/opt/pgsql/bin/postgres' * Given these inputs, the common prefix is '/usr/local/', the tail of * bin_path is 'bin' which does match the last directory component of * my_exec_path, so we would return '/opt/pgsql/share/postgresql' -- 2.38.1 From 6118b4098a50b78ca7437938117cc3ab41b72e08 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 13 Nov 2022 20:52:56 +0100 Subject: [PATCH 2/2] Don't install postmaster symlink anymore Also remove man page. --- doc/src/sgml/ref/postmaster.sgml | 44 -------------------------------- doc/src/sgml/reference.sgml | 1 - src/backend/Makefile | 8 +----- 3 files changed, 1 insertion(+), 52 deletions(-) delete mode 100644 doc/src/sgml/ref/postmaster.sgml diff --git a/doc/src/sgml/ref/postmaster.sgml b/doc/src/sgml/ref/postmaster.sgml deleted file mode 100644 index 7b544ed0b613..000000000000 --- a/doc/src/sgml/ref/postmaster.sgml +++ /dev/null @@ -1,44 +0,0 @@ -<!-- -doc/src/sgml/ref/postmaster.sgml -PostgreSQL documentation ---> - -<refentry id="app-postmaster"> - <indexterm zone="app-postmaster"> - <primary>postmaster</primary> - </indexterm> - - <refmeta> - <refentrytitle><application>postmaster</application></refentrytitle> - <manvolnum>1</manvolnum> - <refmiscinfo>Application</refmiscinfo> - </refmeta> - - <refnamediv> - <refname>postmaster</refname> - <refpurpose><productname>PostgreSQL</productname> database server</refpurpose> - </refnamediv> - - <refsynopsisdiv> - <cmdsynopsis> - <command>postmaster</command> - <arg rep="repeat"><replaceable>option</replaceable></arg> - </cmdsynopsis> - </refsynopsisdiv> - - <refsect1> - <title>Description</title> - - <para> - <command>postmaster</command> is a deprecated alias of <command>postgres</command>. - </para> - </refsect1> - - <refsect1> - <title>See Also</title> - - <para> - <xref linkend="app-postgres"/> - </para> - </refsect1> -</refentry> diff --git a/doc/src/sgml/reference.sgml b/doc/src/sgml/reference.sgml index a3b743e8c1e7..e11b4b613075 100644 --- a/doc/src/sgml/reference.sgml +++ b/doc/src/sgml/reference.sgml @@ -289,7 +289,6 @@ <title>PostgreSQL Server Applications</title> &pgupgrade; &pgwaldump; &postgres; - &postmaster; </reference> diff --git a/src/backend/Makefile b/src/backend/Makefile index 181c217fae4c..ed364543b74e 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -208,12 +208,6 @@ endif install-bin: postgres $(POSTGRES_IMP) installdirs $(INSTALL_PROGRAM) postgres$(X) '$(DESTDIR)$(bindir)/postgres$(X)' -ifneq ($(PORTNAME), win32) - @rm -f '$(DESTDIR)$(bindir)/postmaster$(X)' - ln -s postgres$(X) '$(DESTDIR)$(bindir)/postmaster$(X)' -else - $(INSTALL_PROGRAM) postgres$(X) '$(DESTDIR)$(bindir)/postmaster$(X)' -endif ifeq ($(MAKE_EXPORTS), true) $(INSTALL_DATA) $(POSTGRES_IMP) '$(DESTDIR)$(pkglibdir)/$(POSTGRES_IMP)' $(INSTALL_PROGRAM) $(MKLDEXPORT) '$(DESTDIR)$(pgxsdir)/$(MKLDEXPORT_DIR)/mkldexport.sh' @@ -242,7 +236,7 @@ endif ########################################################################## uninstall: - rm -f '$(DESTDIR)$(bindir)/postgres$(X)' '$(DESTDIR)$(bindir)/postmaster' + rm -f '$(DESTDIR)$(bindir)/postgres$(X)' ifeq ($(MAKE_EXPORTS), true) rm -f '$(DESTDIR)$(pkglibdir)/$(POSTGRES_IMP)' rm -f '$(DESTDIR)$(pgxsdir)/$(MKLDEXPORT_DIR)/mkldexport.sh' -- 2.38.1 Attachments: [text/plain] 0001-Remove-gratuitous-references-to-postmaster-instead-o.patch (2.1K, ../../[email protected]/2-0001-Remove-gratuitous-references-to-postmaster-instead-o.patch) download | inline diff: From 179eaab96dc5215970b2b3485cf77f1a58f9d337 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 13 Nov 2022 20:52:00 +0100 Subject: [PATCH 1/2] Remove gratuitous references to postmaster instead of postgres --- contrib/start-scripts/freebsd | 2 +- contrib/start-scripts/linux | 2 +- src/port/path.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/start-scripts/freebsd b/contrib/start-scripts/freebsd index 3323237a54b4..5e22b2d2f0e4 100644 --- a/contrib/start-scripts/freebsd +++ b/contrib/start-scripts/freebsd @@ -29,7 +29,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin # What to use to start up the postmaster. (If you want the script to wait # until the server has started, you could use "pg_ctl start" here.) -DAEMON="$prefix/bin/postmaster" +DAEMON="$prefix/bin/postgres" # What to use to shut down the postmaster PGCTL="$prefix/bin/pg_ctl" diff --git a/contrib/start-scripts/linux b/contrib/start-scripts/linux index a7757162fc4b..9292855df716 100644 --- a/contrib/start-scripts/linux +++ b/contrib/start-scripts/linux @@ -61,7 +61,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin # What to use to start up the postmaster. (If you want the script to wait # until the server has started, you could use "pg_ctl start" here.) -DAEMON="$prefix/bin/postmaster" +DAEMON="$prefix/bin/postgres" # What to use to shut down the postmaster PGCTL="$prefix/bin/pg_ctl" diff --git a/src/port/path.c b/src/port/path.c index 05fe812f757b..fb64873c7a43 100644 --- a/src/port/path.c +++ b/src/port/path.c @@ -651,7 +651,7 @@ dir_strcmp(const char *s1, const char *s2) * For example: * target_path = '/usr/local/share/postgresql' * bin_path = '/usr/local/bin' - * my_exec_path = '/opt/pgsql/bin/postmaster' + * my_exec_path = '/opt/pgsql/bin/postgres' * Given these inputs, the common prefix is '/usr/local/', the tail of * bin_path is 'bin' which does match the last directory component of * my_exec_path, so we would return '/opt/pgsql/share/postgresql' -- 2.38.1 [text/plain] 0002-Don-t-install-postmaster-symlink-anymore.patch (3.0K, ../../[email protected]/3-0002-Don-t-install-postmaster-symlink-anymore.patch) download | inline diff: From 6118b4098a50b78ca7437938117cc3ab41b72e08 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 13 Nov 2022 20:52:56 +0100 Subject: [PATCH 2/2] Don't install postmaster symlink anymore Also remove man page. --- doc/src/sgml/ref/postmaster.sgml | 44 -------------------------------- doc/src/sgml/reference.sgml | 1 - src/backend/Makefile | 8 +----- 3 files changed, 1 insertion(+), 52 deletions(-) delete mode 100644 doc/src/sgml/ref/postmaster.sgml diff --git a/doc/src/sgml/ref/postmaster.sgml b/doc/src/sgml/ref/postmaster.sgml deleted file mode 100644 index 7b544ed0b613..000000000000 --- a/doc/src/sgml/ref/postmaster.sgml +++ /dev/null @@ -1,44 +0,0 @@ -<!-- -doc/src/sgml/ref/postmaster.sgml -PostgreSQL documentation ---> - -<refentry id="app-postmaster"> - <indexterm zone="app-postmaster"> - <primary>postmaster</primary> - </indexterm> - - <refmeta> - <refentrytitle><application>postmaster</application></refentrytitle> - <manvolnum>1</manvolnum> - <refmiscinfo>Application</refmiscinfo> - </refmeta> - - <refnamediv> - <refname>postmaster</refname> - <refpurpose><productname>PostgreSQL</productname> database server</refpurpose> - </refnamediv> - - <refsynopsisdiv> - <cmdsynopsis> - <command>postmaster</command> - <arg rep="repeat"><replaceable>option</replaceable></arg> - </cmdsynopsis> - </refsynopsisdiv> - - <refsect1> - <title>Description</title> - - <para> - <command>postmaster</command> is a deprecated alias of <command>postgres</command>. - </para> - </refsect1> - - <refsect1> - <title>See Also</title> - - <para> - <xref linkend="app-postgres"/> - </para> - </refsect1> -</refentry> diff --git a/doc/src/sgml/reference.sgml b/doc/src/sgml/reference.sgml index a3b743e8c1e7..e11b4b613075 100644 --- a/doc/src/sgml/reference.sgml +++ b/doc/src/sgml/reference.sgml @@ -289,7 +289,6 @@ <title>PostgreSQL Server Applications</title> &pgupgrade; &pgwaldump; &postgres; - &postmaster; </reference> diff --git a/src/backend/Makefile b/src/backend/Makefile index 181c217fae4c..ed364543b74e 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -208,12 +208,6 @@ endif install-bin: postgres $(POSTGRES_IMP) installdirs $(INSTALL_PROGRAM) postgres$(X) '$(DESTDIR)$(bindir)/postgres$(X)' -ifneq ($(PORTNAME), win32) - @rm -f '$(DESTDIR)$(bindir)/postmaster$(X)' - ln -s postgres$(X) '$(DESTDIR)$(bindir)/postmaster$(X)' -else - $(INSTALL_PROGRAM) postgres$(X) '$(DESTDIR)$(bindir)/postmaster$(X)' -endif ifeq ($(MAKE_EXPORTS), true) $(INSTALL_DATA) $(POSTGRES_IMP) '$(DESTDIR)$(pkglibdir)/$(POSTGRES_IMP)' $(INSTALL_PROGRAM) $(MKLDEXPORT) '$(DESTDIR)$(pgxsdir)/$(MKLDEXPORT_DIR)/mkldexport.sh' @@ -242,7 +236,7 @@ endif ########################################################################## uninstall: - rm -f '$(DESTDIR)$(bindir)/postgres$(X)' '$(DESTDIR)$(bindir)/postmaster' + rm -f '$(DESTDIR)$(bindir)/postgres$(X)' ifeq ($(MAKE_EXPORTS), true) rm -f '$(DESTDIR)$(pkglibdir)/$(POSTGRES_IMP)' rm -f '$(DESTDIR)$(pgxsdir)/$(MKLDEXPORT_DIR)/mkldexport.sh' -- 2.38.1 ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 14:18 Joe Conway <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Joe Conway @ 2022-11-23 14:18 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; pgsql-hackers; Devrim GUNDUZ <[email protected]> On 11/23/22 02:52, Peter Eisentraut wrote: > A little while ago we discussed briefly over in the meson thread whether > we could remove the postmaster symlink [0]. The meson build system > currently does not install a postmaster symlink. (AFAICT, the MSVC > build system does not either.) So if we want to elevate the meson build > system, we either need to add the postmaster symlink, or remove it from > the other build system(s) as well. Seeing that it's been deprecated for > a long time, I propose we just remove it. See attached patches. I am a big +1 on removing the symlink, however it is worth pointing out that the PGDG RPMs still use the symlink in the included systemd service file: 8<---------- ExecStart=/usr/pgsql-15/bin/postmaster -D ${PGDATA} 8<---------- -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 14:28 Devrim Gündüz <[email protected]> parent: Joe Conway <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Devrim Gündüz @ 2022-11-23 14:28 UTC (permalink / raw) To: Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On Wed, 2022-11-23 at 09:18 -0500, Joe Conway wrote: > I am a big +1 on removing the symlink, however it is worth pointing > out > that the PGDG RPMs still use the symlink in the included systemd > service > file: > > 8<---------- > ExecStart=/usr/pgsql-15/bin/postmaster -D ${PGDATA} ...and it helps us to find the "main" process a bit easily. Regards, -- Devrim Gündüz Open Source Solution Architect, Red Hat Certified Engineer Twitter: @DevrimGunduz , @DevrimGunduzTR Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 15:07 Tom Lane <[email protected]> parent: Devrim Gündüz <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Tom Lane @ 2022-11-23 15:07 UTC (permalink / raw) To: Devrim Gündüz <[email protected]>; +Cc: Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Devrim =?ISO-8859-1?Q?G=FCnd=FCz?= <[email protected]> writes: > ...and it helps us to find the "main" process a bit easily. Hmm, that's a nontrivial point perhaps. It's certain that this will break some other people's start scripts too. On the whole, is it really that hard to add the symlink to the meson build? regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 19:50 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 47+ messages in thread From: Andres Freund @ 2022-11-23 19:50 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On 2022-11-23 10:07:49 -0500, Tom Lane wrote: > Devrim =?ISO-8859-1?Q?G=FCnd=FCz?= <[email protected]> writes: > > ...and it helps us to find the "main" process a bit easily. > > Hmm, that's a nontrivial point perhaps. It's certain that this > will break some other people's start scripts too. OTOH, postmaster has been deprecated for ~15 years. > On the whole, is it really that hard to add the symlink to the meson build? No. Meson has a builtin command for it, just not in the meson version we're currently requiring. We can create the symlink ourselves instead. The problem is just detecting systems where we can't symlink and what to fall back to there. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 20:10 Robert Haas <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 2 replies; 47+ messages in thread From: Robert Haas @ 2022-11-23 20:10 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Devrim Gündüz <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Wed, Nov 23, 2022 at 2:50 PM Andres Freund <[email protected]> wrote: > On 2022-11-23 10:07:49 -0500, Tom Lane wrote: > > Devrim =?ISO-8859-1?Q?G=FCnd=FCz?= <[email protected]> writes: > > > ...and it helps us to find the "main" process a bit easily. > > > > Hmm, that's a nontrivial point perhaps. It's certain that this > > will break some other people's start scripts too. > > OTOH, postmaster has been deprecated for ~15 years. Yeah. Also, I don't think it's generally too hard to find the parent process anyway, because at least on my system, the other ones end up with ps display that looks like "postgres: logical replication launcher" or whatever. The main process doesn't set the ps status display, so that's the only one that shows a full path to the executable in the ps status, which is how I usually spot it. That has the advantage that it doesn't matter which name was used to launch it, too. I don't actually care very much whether we get rid of the postmaster symlink or not, but if we aren't going to, we should stop calling it deprecated. If 15 years isn't enough time to remove it, what ever will be? I tend to think it's fairly pointless and perhaps also a bit confusing, because the product is postgres not postmaster and people can reasonably expect the binary name to match the product name. But if we keep it, I don't think anything too dire will happen, either. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 20:32 Joe Conway <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 47+ messages in thread From: Joe Conway @ 2022-11-23 20:32 UTC (permalink / raw) To: Robert Haas <[email protected]>; Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Devrim Gündüz <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On 11/23/22 15:10, Robert Haas wrote: > On Wed, Nov 23, 2022 at 2:50 PM Andres Freund <[email protected]> wrote: >> On 2022-11-23 10:07:49 -0500, Tom Lane wrote: >> > Devrim =?ISO-8859-1?Q?G=FCnd=FCz?= <[email protected]> writes: >> > > ...and it helps us to find the "main" process a bit easily. >> > >> > Hmm, that's a nontrivial point perhaps. It's certain that this >> > will break some other people's start scripts too. >> >> OTOH, postmaster has been deprecated for ~15 years. > > Yeah. Also, I don't think it's generally too hard to find the parent > process anyway, because at least on my system, the other ones end up > with ps display that looks like "postgres: logical replication > launcher" or whatever. The main process doesn't set the ps status > display, so that's the only one that shows a full path to the > executable in the ps status, which is how I usually spot it. That has > the advantage that it doesn't matter which name was used to launch it, > too. Same here > I don't actually care very much whether we get rid of the postmaster > symlink or not, but if we aren't going to, we should stop calling it > deprecated. If 15 years isn't enough time to remove it, what ever will > be? I tend to think it's fairly pointless and perhaps also a bit > confusing, because the product is postgres not postmaster and people > can reasonably expect the binary name to match the product name. But > if we keep it, I don't think anything too dire will happen, either. FWIW, the reason I took note of the postmaster symlink in the first place a few years ago was because selinux treats execution of programs from symlinks differently than from actual files. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 20:48 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 47+ messages in thread From: Tom Lane @ 2022-11-23 20:48 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > On 2022-11-23 10:07:49 -0500, Tom Lane wrote: >> On the whole, is it really that hard to add the symlink to the meson build? > No. Meson has a builtin command for it, just not in the meson version we're > currently requiring. We can create the symlink ourselves instead. The problem > is just detecting systems where we can't symlink and what to fall back to > there. This isn't a hill I want to die on, either way. But "it's a bit more complicated in meson" seems like a poor reason for changing the user-visible installed fileset. regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-23 21:08 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Andres Freund @ 2022-11-23 21:08 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On 2022-11-23 15:48:04 -0500, Tom Lane wrote: > Andres Freund <[email protected]> writes: > > On 2022-11-23 10:07:49 -0500, Tom Lane wrote: > >> On the whole, is it really that hard to add the symlink to the meson build? > > > No. Meson has a builtin command for it, just not in the meson version we're > > currently requiring. We can create the symlink ourselves instead. The problem > > is just detecting systems where we can't symlink and what to fall back to > > there. > > This isn't a hill I want to die on, either way. But "it's a bit > more complicated in meson" seems like a poor reason for changing > the user-visible installed fileset. I wouldn't even have thought about proposing dropping the symlink if it hadn't been deprecated forever, and I suspect Peter wouldn't have either... I think this is a bit more more complicated than "changing the user-visible installed fileset" because we didn't have logic to create 'postmaster' on windows before, afaik the only OS without reliable symlink support. Anyway, my current thinking is to have dumb OS dependent behaviour and create a symlink everywhere but windows, where we'd just copy the file. Or we could just continue to not install 'postmaster' on windows, because of the potential confusion that 'postmaster.exe' differing from 'postgres.exe' could cause. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: drop postmaster symlink @ 2022-11-24 00:15 Daniel Gustafsson <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 47+ messages in thread From: Daniel Gustafsson @ 2022-11-24 00:15 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Devrim Gündüz <[email protected]>; Joe Conway <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers > On 23 Nov 2022, at 21:10, Robert Haas <[email protected]> wrote: > I don't actually care very much whether we get rid of the postmaster > symlink or not, but if we aren't going to, we should stop calling it > deprecated. If 15 years isn't enough time to remove it, what ever will > be? +1. If we actively add support for something then it isn't really all that deprecated IMHO. -- Daniel Gustafsson https://vmware.com/ ^ permalink raw reply [nested|flat] 47+ messages in thread
* [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 12:21 Josef Šimánek <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Josef Šimánek @ 2023-12-15 12:21 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hello! Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list of ideas for PostgreSQL (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a quick patch to do SQL syntax validation. It is also heavily inspired by the "ruby -c" command, useful to check syntax of Ruby programs without executing them. For now, to keep it simple and to open discussion, I have added new "--syntax" option into "postgres" command, since it is currently the only one using needed parser dependency (at least per my understanding). I tried to add this into psql or separate pg_syntax commands, but parser is not exposed in "postgres_fe.h" and including backend into those tools would not make most likely sense. Also syntax could vary per backend, it makes sense to keep it in there. It expects input on STDIN, prints out error if any and prints out summary message (Valid SQL/Invalid SQL). On valid input it exits with 0 (success), otherwise it exits with 1 (error). Example usage: $ echo "SELECT 1" | src/backend/postgres --syntax Valid SQL $ echo "SELECT 1abc" | src/backend/postgres --syntax ERROR: trailing junk after numeric literal at or near "1a" at character 8 Invalid SQL $ cat ../src/test/regress/sql/alter_operator.sql | src/backend/postgres --syntax Valid SQL $ cat ../src/test/regress/sql/advisory_lock.sql | src/backend/postgres --syntax ERROR: syntax error at or near "\" at character 99 Invalid SQL This could be useful for manual script checks, automated script checks and code editor integrations. Notice it just parses the SQL, it doesn't detect any "runtime" problems like unexisting table names, etc. I have various ideas around this (like exposing similar functionality directly in SQL using custom function like pg_check_syntax), but I would like to get some feedback first. What do you think? enhnace PS: I wasn't able to find any automated tests for "postgres" command to enhance with, are there any? PS2: Patch could be found at https://github.com/simi/postgres/pull/8 as well. Attachments: [text/x-patch] 01-postgres-check.patch (2.6K, ../../CAFp7QwqJmeYHdXB0fiZivnQaYG8A6_fpQmNRw_v5REY=cJ7GTQ@mail.gmail.com/2-01-postgres-check.patch) download | inline diff: From 6ec6cc599678c0ac76f4559039ff3399f843b9b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= <[email protected]> Date: Fri, 15 Dec 2023 13:00:23 +0100 Subject: [PATCH] Add --syntax option to postgres. - it validates SQL on STDIN and reports back if valid/invalid --- src/backend/main/main.c | 45 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/backend/main/main.c b/src/backend/main/main.c index ed11e8be7fab..eb7266f9d5d3 100644 --- a/src/backend/main/main.c +++ b/src/backend/main/main.c @@ -40,6 +40,7 @@ #include "utils/memutils.h" #include "utils/pg_locale.h" #include "utils/ps_status.h" +#include "parser/parser.h" const char *progname; @@ -50,6 +51,7 @@ static void startup_hacks(const char *progname); static void init_locale(const char *categoryname, int category, const char *locale); static void help(const char *progname); static void check_root(const char *progname); +static void check_syntax(void); /* @@ -153,6 +155,10 @@ main(int argc, char *argv[]) fputs(PG_BACKEND_VERSIONSTR, stdout); exit(0); } + if (strcmp(argv[1], "--syntax") == 0) + { + check_syntax(); + } /* * In addition to the above, we allow "--describe-config" and "-C var" @@ -347,6 +353,7 @@ help(const char *progname) printf(_(" -s show statistics after each query\n")); printf(_(" -S WORK-MEM set amount of memory for sorts (in kB)\n")); printf(_(" -V, --version output version information, then exit\n")); + printf(_(" --syntax checks SQL on STDIN is valid, then exit\n")); printf(_(" --NAME=VALUE set run-time parameter\n")); printf(_(" --describe-config describe configuration parameters, then exit\n")); printf(_(" -?, --help show this help, then exit\n")); @@ -422,6 +429,44 @@ check_root(const char *progname) #endif /* WIN32 */ } +static void +check_syntax() { + List *parsetree_list; + bool valid = false; + char buffer[1024 * 1024]; + char ch; + int i = 0; + + // TODO: check for buffer overflow + while ((ch = getchar()) != EOF) + { + buffer[i] = ch; + i++; + } + + PG_TRY(); + { + parsetree_list = raw_parser(buffer, RAW_PARSE_DEFAULT); + valid = true; + } + PG_CATCH(); + { + EmitErrorReport(); + FlushErrorState(); + valid = false; + } + PG_END_TRY(); + + // TODO: translate output + if (valid && parsetree_list) { + printf("Valid SQL\n"); + exit(0); + } else { + printf("Invalid SQL\n"); + exit(1); + } +} + /* * At least on linux, set_ps_display() breaks /proc/$pid/environ. The * sanitizer library uses /proc/$pid/environ to implement getenv() as it wants ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 13:09 Laurenz Albe <[email protected]> parent: Josef Šimánek <[email protected]> 0 siblings, 2 replies; 47+ messages in thread From: Laurenz Albe @ 2023-12-15 13:09 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: > Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list > of ideas for PostgreSQL > (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a > quick patch to do SQL syntax validation. > > What do you think? I like the idea. But what will happen if the SQL statement references tables or other objects, since we have no database? Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 13:14 Josef Šimánek <[email protected]> parent: Laurenz Albe <[email protected]> 1 sibling, 1 reply; 47+ messages in thread From: Josef Šimánek @ 2023-12-15 13:14 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Dne pá 15. 12. 2023 14:09 uživatel Laurenz Albe <[email protected]> napsal: > On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: > > Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list > > of ideas for PostgreSQL > > (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a > > quick patch to do SQL syntax validation. > > > > What do you think? > > I like the idea. But what will happen if the SQL statement references > tables or other objects, since we have no database? > It checks just the identifier is valid from parser perspective, like it is valid table name. > Yours, > Laurenz Albe > ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 13:19 Pavel Stehule <[email protected]> parent: Josef Šimánek <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Pavel Stehule @ 2023-12-15 13:19 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; +Cc: Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> Dne pá 15. 12. 2023 14:14 uživatel Josef Šimánek <[email protected]> napsal: > > > Dne pá 15. 12. 2023 14:09 uživatel Laurenz Albe <[email protected]> > napsal: > >> On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: >> > Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list >> > of ideas for PostgreSQL >> > (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a >> > quick patch to do SQL syntax validation. >> > >> > What do you think? >> >> I like the idea. But what will happen if the SQL statement references >> tables or other objects, since we have no database? >> > > It checks just the identifier is valid from parser perspective, like it is > valid table name. > There can by two levels, like plpgsql or like pllgsql_check Regards Pavel > > >> Yours, >> Laurenz Albe >> > ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 14:50 Tom Lane <[email protected]> parent: Laurenz Albe <[email protected]> 1 sibling, 2 replies; 47+ messages in thread From: Tom Lane @ 2023-12-15 14:50 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: Josef Šimánek <[email protected]>; PostgreSQL Hackers <[email protected]> Laurenz Albe <[email protected]> writes: > On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: >> Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list >> of ideas for PostgreSQL >> (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a >> quick patch to do SQL syntax validation. >> >> What do you think? > I like the idea. But what will happen if the SQL statement references > tables or other objects, since we have no database? This seems like a fairly useless wart to me. What does it do that you can't do better with existing facilities (psql etc)? In the big picture a command line switch in the postgres executable doesn't feel like the right place for this. There's no good reason to assume that the server executable will be installed where you want this capability; not to mention the possibility of version skew between that executable and whatever installation you're actually running on. Another thing I don't like is that this exposes to the user what ought to be purely an implementation detail, namely the division of labor between gram.y (raw_parser()) and the rest of the parser. There are checks that a user would probably see as "syntax checks" that don't happen in gram.y, and conversely there are some things we reject there that seem more like semantic than syntax issues. regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 15:05 Josef Šimánek <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 47+ messages in thread From: Josef Šimánek @ 2023-12-15 15:05 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> pá 15. 12. 2023 v 15:50 odesílatel Tom Lane <[email protected]> napsal: > > Laurenz Albe <[email protected]> writes: > > On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: > >> Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list > >> of ideas for PostgreSQL > >> (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a > >> quick patch to do SQL syntax validation. > >> > >> What do you think? > > > I like the idea. But what will happen if the SQL statement references > > tables or other objects, since we have no database? > > This seems like a fairly useless wart to me. What does it do that > you can't do better with existing facilities (psql etc)? Per my experience, this is mostly useful during development to catch syntax errors super early. For SELECT/INSERT/UPDATE/DELETE queries, it is usually enough to prepend with EXPLAIN and run. But EXPLAIN doesn't support all SQL like DDL statements. Let's say I have a long SQL script I'm working on and there is typo in the middle like ALTERR instead of ALTER. Is there any simple way to detect this without actually running the statement in psql or other existing facilities? This check could be simply combined with editor capabilities to be run on each SQL file save to get quick feedback on this kind of mistakes for great developer experience. > In the big picture a command line switch in the postgres executable > doesn't feel like the right place for this. There's no good reason > to assume that the server executable will be installed where you want > this capability; not to mention the possibility of version skew > between that executable and whatever installation you're actually > running on. This is mostly intended for SQL developers and CI systems where PostgreSQL backend is usually installed and in the actual version needed. I agree postgres is not the best place (even it makes partially sense to me), but as I mentioned, I wasn't able to craft a quick patch with a better place to put this in. What would you recommend? Separate executable like pg_syntaxcheck? > Another thing I don't like is that this exposes to the user what ought > to be purely an implementation detail, namely the division of labor > between gram.y (raw_parser()) and the rest of the parser. There are > checks that a user would probably see as "syntax checks" that don't > happen in gram.y, and conversely there are some things we reject there > that seem more like semantic than syntax issues. I have no big insight into SQL parsing. Can you please share examples of given concerns? Is there anything better than raw_parser() for this purpose? > regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 15:16 David G. Johnston <[email protected]> parent: Josef Šimánek <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: David G. Johnston @ 2023-12-15 15:16 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; +Cc: Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Dec 15, 2023 at 8:05 AM Josef Šimánek <[email protected]> wrote: > pá 15. 12. 2023 v 15:50 odesílatel Tom Lane <[email protected]> napsal: > > > > Laurenz Albe <[email protected]> writes: > > > On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: > > >> Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list > > >> of ideas for PostgreSQL > > >> (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted > a > > >> quick patch to do SQL syntax validation. > > >> > > >> What do you think? > > > > > I like the idea. But what will happen if the SQL statement references > > > tables or other objects, since we have no database? > > > > This seems like a fairly useless wart to me. What does it do that > > you can't do better with existing facilities (psql etc)? > > Per my experience, this is mostly useful during development to catch > syntax errors super early. I'd suggest helping this project get production capable since it already is trying to integrate with the development ecosystem you describe here. https://github.com/supabase/postgres_lsp I agree that developing this as a new executable for the purpose is needed in order to best conform to existing conventions. David J. ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 15:20 Josef Šimánek <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Josef Šimánek @ 2023-12-15 15:20 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> pá 15. 12. 2023 v 16:16 odesílatel David G. Johnston <[email protected]> napsal: > > On Fri, Dec 15, 2023 at 8:05 AM Josef Šimánek <[email protected]> wrote: >> >> pá 15. 12. 2023 v 15:50 odesílatel Tom Lane <[email protected]> napsal: >> > >> > Laurenz Albe <[email protected]> writes: >> > > On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: >> > >> Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list >> > >> of ideas for PostgreSQL >> > >> (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a >> > >> quick patch to do SQL syntax validation. >> > >> >> > >> What do you think? >> > >> > > I like the idea. But what will happen if the SQL statement references >> > > tables or other objects, since we have no database? >> > >> > This seems like a fairly useless wart to me. What does it do that >> > you can't do better with existing facilities (psql etc)? >> >> Per my experience, this is mostly useful during development to catch >> syntax errors super early. > > > I'd suggest helping this project get production capable since it already is trying to integrate with the development ecosystem you describe here. > > https://github.com/supabase/postgres_lsp Indeed LSP is the one of the targets of this feature. Currently it is using https://github.com/pganalyze/libpg_query under the hood probably because of the same reasons I have described (parser is not available in public APIs of postgres_fe.h or libpq). Exposing basic parser capabilities in postgres binary itself and also on SQL level by pg_check_syntax function can prevent the need of "hacking" pg parser to be accessible outside of server binary. > I agree that developing this as a new executable for the purpose is needed in order to best conform to existing conventions. > > David J. > ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 15:31 David G. Johnston <[email protected]> parent: Josef Šimánek <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: David G. Johnston @ 2023-12-15 15:31 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; +Cc: Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Dec 15, 2023 at 8:20 AM Josef Šimánek <[email protected]> wrote: > (parser is not available > in public APIs of postgres_fe.h or libpq). > What about building "libpg" that does expose and exports some public APIs for the parser? We can include a reference CLI implementation for basic usage of the functionality while leaving the actual language server project outside of core. David J. ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2023-12-15 15:38 Josef Šimánek <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Josef Šimánek @ 2023-12-15 15:38 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> pá 15. 12. 2023 v 16:32 odesílatel David G. Johnston <[email protected]> napsal: > > On Fri, Dec 15, 2023 at 8:20 AM Josef Šimánek <[email protected]> wrote: >> >> (parser is not available >> in public APIs of postgres_fe.h or libpq). > > > What about building "libpg" that does expose and exports some public APIs for the parser? We can include a reference CLI implementation for basic usage of the functionality while leaving the actual language server project outside of core. Language server (LSP) can just benefit from this feature, but it doesn't cover all possibilities since LSP is meant for one purpose -> run in developer's code editor. Actual syntax check is more generic, able to cover CI checks and more. I would not couple this feature and LSP, LSP can just benefit from it (and it is usually built in a way that uses other tools and packs them into LSP). Exposing this kind of SQL check doesn't mean something LSP related being implemented in core. LSP can just benefit from this. Exposing parser to libpg seems good idea, but I'm not sure how simple that could be done since during my journey I have found out there are a lot of dependencies which are not present in usual frontend code per my understanding like memory contexts and removal of those (decoupling) would be huge project IMHO. > David J. ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-02-25 22:24 Tomas Vondra <[email protected]> parent: Josef Šimánek <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Tomas Vondra @ 2024-02-25 22:24 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; David G. Johnston <[email protected]>; +Cc: Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On 12/15/23 16:38, Josef Šimánek wrote: > pá 15. 12. 2023 v 16:32 odesílatel David G. Johnston > <[email protected]> napsal: >> >> On Fri, Dec 15, 2023 at 8:20 AM Josef Šimánek <[email protected]> wrote: >>> >>> (parser is not available >>> in public APIs of postgres_fe.h or libpq). >> >> >> What about building "libpg" that does expose and exports some public APIs for the parser? We can include a reference CLI implementation for basic usage of the functionality while leaving the actual language server project outside of core. > > Language server (LSP) can just benefit from this feature, but it > doesn't cover all possibilities since LSP is meant for one purpose -> > run in developer's code editor. Actual syntax check is more generic, > able to cover CI checks and more. I would not couple this feature and > LSP, LSP can just benefit from it (and it is usually built in a way > that uses other tools and packs them into LSP). Exposing this kind of > SQL check doesn't mean something LSP related being implemented in > core. LSP can just benefit from this. > I don't know enough about LSP to have a good idea how to implement this for PG, but my assumption would be we'd have some sort of library exposing "parser" to frontend tools, and also an in-core binary using that library (say, src/bin/pg_syntax_check). And LSP would use that parser library too ... I think there's about 0% chance we'd add this to "postgres" binary. > Exposing parser to libpg seems good idea, but I'm not sure how simple > that could be done since during my journey I have found out there are > a lot of dependencies which are not present in usual frontend code per > my understanding like memory contexts and removal of those > (decoupling) would be huge project IMHO. > You're right the grammar/parser expects a lot of backend infrastructure, so making it available to frontend is going to be challenging. But I doubt there's a better way than starting with gram.y and either removing or adding the missing pieces (maybe only a mock version of it). I'm not a bison expert, but considering your goal seems to be a basic syntax check, maybe you could simply rip out most of the bits depending on backend stuff, or maybe replace them with some trivial no-op code? But as Tom mentioned, the question is how far gram.y gets you. There's plenty of ereport(ERROR) calls in src/backend/parser/*.c our users might easily consider as parse errors ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 17:42 Robert Haas <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: Robert Haas @ 2024-05-15 17:42 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Josef Šimánek <[email protected]>; David G. Johnston <[email protected]>; Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, Feb 25, 2024 at 5:24 PM Tomas Vondra <[email protected]> wrote: > I think there's about 0% chance we'd add this to "postgres" binary. Several people have taken this position, so I'm going to mark https://commitfest.postgresql.org/48/4704/ as Rejected. My own opinion is that syntax checking is a useful thing to expose, but I don't believe that this is a useful way to expose it. I think this comment that Tom made upthread is right on target: # Another thing I don't like is that this exposes to the user what ought # to be purely an implementation detail, namely the division of labor # between gram.y (raw_parser()) and the rest of the parser. There are # checks that a user would probably see as "syntax checks" that don't # happen in gram.y, and conversely there are some things we reject there # that seem more like semantic than syntax issues. I think that what that means in practice is that, while this patch may seem to give reasonable results in simple tests, as soon as you try to do slightly more complicated things with it, it's going to give weird results, either failing to flag things that you'd expect it to flag, or flagging things you'd expect it not to flag. Fixing that would be either impossible or a huge amount of work depending on your point of view. If you take the point of view that we need to adjust things so that the raw parser reports all the things that ought to be reported by a tool like this and none of the things that it shouldn't, then it's probably just a huge amount of work. If you take the point of view that what goes into the raw parser and what goes into parse analysis ought to be an implementation decision untethered to what a tool like this ought to report, then fixing the problems would be impossible, or at least, it would amount to throwing away this patch and starting over. I think the latter point of view, which Tom has already taken, would be the more prevalent view among hackers by far, but even if the former view prevailed, who is going to do all that work? I strongly suspect that doing something useful in this area requires about two orders of magnitude more code than are included in this patch, and a completely different design. If it actually worked well to do something this simple, somebody probably would have done it already. In fact, there are already tools out there for validation, like https://github.com/okbob/plpgsql_check for example. That tool doesn't do exactly the same thing as this patch is trying to do, but it does do other kinds of validation, and it's 19k lines of code, vs. the 45 lines of code in this patch, which I think reinforces the point that you need to do something much more complicated than this to create real value. Also, the patch as proposed, besides being 45 lines, also has zero lines of comments, no test cases, no documentation, and doesn't follow the PostgreSQL coding standards. I'm not saying that to be mean, nor am I suggesting that Josef should go fix that stuff. It's perfectly reasonable to propose a small patch without a lot of research to see what people think -- but now we have the answer to that question: people think it won't work. So Josef should now decide to either give up, or try a new approach, or if he's really sure that all the feedback that has been given so far is completely wrong, he can try to demonstrate that the patch does all kinds of wonderful things with very few disadvantages. But I think if he does that last, he's going to find that it's not really possible, because I'm pretty sure that Tom is right. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 18:12 Josef Šimánek <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 2 replies; 47+ messages in thread From: Josef Šimánek @ 2024-05-15 18:12 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> st 15. 5. 2024 v 19:43 odesílatel Robert Haas <[email protected]> napsal: > > On Sun, Feb 25, 2024 at 5:24 PM Tomas Vondra > <[email protected]> wrote: > > I think there's about 0% chance we'd add this to "postgres" binary. > > Several people have taken this position, so I'm going to mark > https://commitfest.postgresql.org/48/4704/ as Rejected. > > My own opinion is that syntax checking is a useful thing to expose, > but I don't believe that this is a useful way to expose it. I think > this comment that Tom made upthread is right on target: > > # Another thing I don't like is that this exposes to the user what ought > # to be purely an implementation detail, namely the division of labor > # between gram.y (raw_parser()) and the rest of the parser. There are > # checks that a user would probably see as "syntax checks" that don't > # happen in gram.y, and conversely there are some things we reject there > # that seem more like semantic than syntax issues. > > I think that what that means in practice is that, while this patch may > seem to give reasonable results in simple tests, as soon as you try to > do slightly more complicated things with it, it's going to give weird > results, either failing to flag things that you'd expect it to flag, > or flagging things you'd expect it not to flag. Fixing that would be > either impossible or a huge amount of work depending on your point of > view. If you take the point of view that we need to adjust things so > that the raw parser reports all the things that ought to be reported > by a tool like this and none of the things that it shouldn't, then > it's probably just a huge amount of work. If you take the point of > view that what goes into the raw parser and what goes into parse > analysis ought to be an implementation decision untethered to what a > tool like this ought to report, then fixing the problems would be > impossible, or at least, it would amount to throwing away this patch > and starting over. I think the latter point of view, which Tom has > already taken, would be the more prevalent view among hackers by far, > but even if the former view prevailed, who is going to do all that > work? > > I strongly suspect that doing something useful in this area requires > about two orders of magnitude more code than are included in this > patch, and a completely different design. If it actually worked well > to do something this simple, somebody probably would have done it > already. In fact, there are already tools out there for validation, > like https://github.com/okbob/plpgsql_check for example. That tool > doesn't do exactly the same thing as this patch is trying to do, but > it does do other kinds of validation, and it's 19k lines of code, vs. > the 45 lines of code in this patch, which I think reinforces the point > that you need to do something much more complicated than this to > create real value. > > Also, the patch as proposed, besides being 45 lines, also has zero > lines of comments, no test cases, no documentation, and doesn't follow > the PostgreSQL coding standards. I'm not saying that to be mean, nor > am I suggesting that Josef should go fix that stuff. It's perfectly > reasonable to propose a small patch without a lot of research to see > what people think -- but now we have the answer to that question: > people think it won't work. So Josef should now decide to either give > up, or try a new approach, or if he's really sure that all the > feedback that has been given so far is completely wrong, he can try to > demonstrate that the patch does all kinds of wonderful things with > very few disadvantages. But I think if he does that last, he's going > to find that it's not really possible, because I'm pretty sure that > Tom is right. I'm totally OK to mark this as rejected and indeed 45 lines were just minimal patch to create PoC (I have coded this during last PGConf.eu lunch break) and mainly to start discussion. I'm not sure everyone in this thread understands the reason for this patch, which is clearly my fault, since I have failed to explain. Main idea is to make a tool to validate query can be parsed, that's all. Similar to running "EXPLAIN query", but not caring about the result and not caring about the DB structure (ignoring missing tables, ...), just checking it was successfully executed. This definitely belongs to the server side and not to the client side, it is just a tool to validate that for this running PostgreSQL backend it is a "parseable" query. I'm not giving up on this, but I hear there are various problems to explore. If I understand it well, just running the parser to query doesn't guarantee the query is valid, since it can fail later for various reasons (I mean other than missing table, ...). I wasn't aware of that. Also exposing this inside postgres binary seems controversial. I had an idea to expose parser result at SQL level with a new command (similar to EXPLAIN), but you'll need running PostgreSQL backend to be able to use this capability, which is against one of the original ideas. On the otherside PostgreSQL exposes a lot of "meta" functionality already and this could be a nice addition. I'll revisit the discussion again and try to submit another try once I'll get more context and experience. Thanks everyone for constructive comments! > -- > Robert Haas > EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 18:13 Josef Šimánek <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 47+ messages in thread From: Josef Šimánek @ 2024-05-15 18:13 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> pá 15. 12. 2023 v 15:50 odesílatel Tom Lane <[email protected]> napsal: > > Laurenz Albe <[email protected]> writes: > > On Fri, 2023-12-15 at 13:21 +0100, Josef Šimánek wrote: > >> Inspired by Simon Riggs' keynote talk at PGCounf.eu 2023 sharing list > >> of ideas for PostgreSQL > >> (https://riggs.business/blog/f/postgresql-todo-2023), I have crafted a > >> quick patch to do SQL syntax validation. > >> > >> What do you think? > > > I like the idea. But what will happen if the SQL statement references > > tables or other objects, since we have no database? > > This seems like a fairly useless wart to me. this hurts :'( > > In the big picture a command line switch in the postgres executable > doesn't feel like the right place for this. There's no good reason > to assume that the server executable will be installed where you want > this capability; not to mention the possibility of version skew > between that executable and whatever installation you're actually > running on. > > Another thing I don't like is that this exposes to the user what ought > to be purely an implementation detail, namely the division of labor > between gram.y (raw_parser()) and the rest of the parser. There are > checks that a user would probably see as "syntax checks" that don't > happen in gram.y, and conversely there are some things we reject there > that seem more like semantic than syntax issues. > > regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 18:39 Tom Lane <[email protected]> parent: Josef Šimánek <[email protected]> 1 sibling, 4 replies; 47+ messages in thread From: Tom Lane @ 2024-05-15 18:39 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; +Cc: Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> =?UTF-8?B?Sm9zZWYgxaBpbcOhbmVr?= <[email protected]> writes: > I'm not sure everyone in this thread understands the reason for this > patch, which is clearly my fault, since I have failed to explain. Main > idea is to make a tool to validate query can be parsed, that's all. > Similar to running "EXPLAIN query", but not caring about the result > and not caring about the DB structure (ignoring missing tables, ...), > just checking it was successfully executed. This definitely belongs to > the server side and not to the client side, it is just a tool to > validate that for this running PostgreSQL backend it is a "parseable" > query. The thing that was bothering me most about this is that I don't understand why that's a useful check. If I meant to type UPDATE mytab SET mycol = 42; and instead I type UPDATEE mytab SET mycol = 42; your proposed feature would catch that; great. But if I type UPDATE mytabb SET mycol = 42; it won't. How does that make sense? I'm not entirely sure where to draw the line about what a "syntax check" should catch, but this seems a bit south of what I'd want in a syntax-checking editor. BTW, if you do feel that a pure grammar check is worthwhile, you should look at the ecpg preprocessor, which does more or less that with the SQL portions of its input. ecpg could be a better model to follow because it doesn't bring all the dependencies of the server and so is much more likely to appear in a client-side installation. It's kind of an ugly, unmaintained mess at the moment, sadly. The big knock on doing this client-side is that there might be version skew compared to the server you're using --- but if you are not doing anything beyond a grammar-level check then your results are pretty approximate anyway, ISTM. We've not heard anything suggesting that version skew is a huge problem for ecpg users. regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 18:49 [email protected] parent: Tom Lane <[email protected]> 3 siblings, 1 reply; 47+ messages in thread From: [email protected] @ 2024-05-15 18:49 UTC (permalink / raw) To: Tom Lane <[email protected]>; Josef Šimánek <[email protected]>; +Cc: Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> Tom Lane: > The thing that was bothering me most about this is that I don't > understand why that's a useful check. If I meant to type > > UPDATE mytab SET mycol = 42; > > and instead I type > > UPDATEE mytab SET mycol = 42; > > your proposed feature would catch that; great. But if I type > > UPDATE mytabb SET mycol = 42; > > it won't. How does that make sense? I'm not entirely sure where > to draw the line about what a "syntax check" should catch, but this > seems a bit south of what I'd want in a syntax-checking editor. > > BTW, if you do feel that a pure grammar check is worthwhile, you > should look at the ecpg preprocessor, which does more or less that > with the SQL portions of its input. ecpg could be a better model > to follow because it doesn't bring all the dependencies of the server > and so is much more likely to appear in a client-side installation. > It's kind of an ugly, unmaintained mess at the moment, sadly. Would working with ecpg allow to get back a parse tree of the query to do stuff with that? This is really what is missing for the ecosystem. A libpqparser for tools to use: Formatters, linters, query rewriters, simple syntax checkers... they are all missing access to postgres' own parser. Best, Wolfgang ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:00 Josef Šimánek <[email protected]> parent: Tom Lane <[email protected]> 3 siblings, 0 replies; 47+ messages in thread From: Josef Šimánek @ 2024-05-15 19:00 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> st 15. 5. 2024 v 20:39 odesílatel Tom Lane <[email protected]> napsal: > > =?UTF-8?B?Sm9zZWYgxaBpbcOhbmVr?= <[email protected]> writes: > > I'm not sure everyone in this thread understands the reason for this > > patch, which is clearly my fault, since I have failed to explain. Main > > idea is to make a tool to validate query can be parsed, that's all. > > Similar to running "EXPLAIN query", but not caring about the result > > and not caring about the DB structure (ignoring missing tables, ...), > > just checking it was successfully executed. This definitely belongs to > > the server side and not to the client side, it is just a tool to > > validate that for this running PostgreSQL backend it is a "parseable" > > query. > > The thing that was bothering me most about this is that I don't > understand why that's a useful check. If I meant to type > > UPDATE mytab SET mycol = 42; > > and instead I type > > UPDATEE mytab SET mycol = 42; > > your proposed feature would catch that; great. But if I type > > UPDATE mytabb SET mycol = 42; > > it won't. How does that make sense? I'm not entirely sure where > to draw the line about what a "syntax check" should catch, but this > seems a bit south of what I'd want in a syntax-checking editor. This is exactly where the line is drawn. My motivation is not to use this feature for syntax check in editor (even could be used to find those banalities). I'm looking to improve automation to be able to detect those banalities as early as possible. Let's say there is complex CI automation configuring and starting PostgreSQL backend, loading some data, ... and in the end all this is useless, since there is this kind of simple mistake like UPDATEE. I would like to detect this problem as early as possible and stop the complex CI pipeline to save time and also to save resources (= money) by failing super-early and reporting back. This kind of mistake could be simply introduced by like wrongly resolved git conflict, human typing error ... This kind of mistake (typo, ...) can be usually spotted super early. In compiled languages during compilation, in interpreted languages (like Ruby) at program start (since code is parsed as one of the first steps). There is no such early detection possible for PostgreSQL currently IMHO. > BTW, if you do feel that a pure grammar check is worthwhile, you > should look at the ecpg preprocessor, which does more or less that > with the SQL portions of its input. ecpg could be a better model > to follow because it doesn't bring all the dependencies of the server > and so is much more likely to appear in a client-side installation. > It's kind of an ugly, unmaintained mess at the moment, sadly. > > The big knock on doing this client-side is that there might be > version skew compared to the server you're using --- but if you > are not doing anything beyond a grammar-level check then your > results are pretty approximate anyway, ISTM. We've not heard > anything suggesting that version skew is a huge problem for > ecpg users. Thanks for the info, I'll check. > regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:01 Tom Lane <[email protected]> parent: [email protected] 0 siblings, 1 reply; 47+ messages in thread From: Tom Lane @ 2024-05-15 19:01 UTC (permalink / raw) To: [email protected]; +Cc: Josef Šimánek <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> [email protected] writes: > Tom Lane: >> BTW, if you do feel that a pure grammar check is worthwhile, you >> should look at the ecpg preprocessor, which does more or less that >> with the SQL portions of its input. > Would working with ecpg allow to get back a parse tree of the query to > do stuff with that? No, ecpg isn't interested in building a syntax tree. > This is really what is missing for the ecosystem. A libpqparser for > tools to use: Formatters, linters, query rewriters, simple syntax > checkers... they are all missing access to postgres' own parser. To get to that, you'd need some kind of agreement on what the syntax tree is. I doubt our existing implementation would be directly useful to very many tools, and even if it is, do they want to track constant version-to-version changes? regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:03 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 3 siblings, 1 reply; 47+ messages in thread From: Robert Haas @ 2024-05-15 19:03 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Josef Šimánek <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, May 15, 2024 at 2:39 PM Tom Lane <[email protected]> wrote: > The thing that was bothering me most about this is that I don't > understand why that's a useful check. If I meant to type > > UPDATE mytab SET mycol = 42; > > and instead I type > > UPDATEE mytab SET mycol = 42; > > your proposed feature would catch that; great. But if I type > > UPDATE mytabb SET mycol = 42; > > it won't. How does that make sense? I'm not entirely sure where > to draw the line about what a "syntax check" should catch, but this > seems a bit south of what I'd want in a syntax-checking editor. I don't agree with this, actually. The first wrong example can never be valid, while the second one can be valid given the right table definitions. That lines up quite nicely with the distinction between parsing and parse analysis. There is a problem with actually getting all the way there, I'm fairly sure, because we've got thousands of lines of gram.y and thousands of lines of parse analysis code that weren't all written with the idea of making a crisp distinction here. For example, I'd like both EXPLAIN (WAFFLES) SELECT 1 and EXPLAIN WAFFLES SELECT 1 to be flagged as syntactically invalid, and with things as they are that would not happen. Even for plannable statements I would not be at all surprised to hear that there are a bunch of corner cases that we'd get wrong. But I don't understand the idea that the concept doesn't make sense. I think it is perfectly reasonable to imagine a world in which the initial parsing takes care of reporting everything that can be determined by static analysis without knowing anything about catalog contents, and parse analysis checks all the things that require catalog access, and you can run the first set of checks and then decide whether to proceed further. I think if I were designing a new system from scratch, I'd definitely want to set it up that way, and I think moving our existing system in that direction would probably let us clean up a variety of warts along the way. Really, the only argument I see against it is that getting from where we are to there would be a gigantic amount of work for the value we'd derive. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:05 Robert Haas <[email protected]> parent: Josef Šimánek <[email protected]> 1 sibling, 1 reply; 47+ messages in thread From: Robert Haas @ 2024-05-15 19:05 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, May 15, 2024 at 2:12 PM Josef Šimánek <[email protected]> wrote: > I'm totally OK to mark this as rejected and indeed 45 lines were just > minimal patch to create PoC (I have coded this during last PGConf.eu > lunch break) and mainly to start discussion. Thanks for understanding. > I'm not sure everyone in this thread understands the reason for this > patch, which is clearly my fault, since I have failed to explain. Main > idea is to make a tool to validate query can be parsed, that's all. > Similar to running "EXPLAIN query", but not caring about the result > and not caring about the DB structure (ignoring missing tables, ...), > just checking it was successfully executed. This definitely belongs to > the server side and not to the client side, it is just a tool to > validate that for this running PostgreSQL backend it is a "parseable" > query. I don't think it's at all obvious that this belongs on the server side rather than the client side. I think it could be done in either place, or both. I just think we don't have the infrastructure to do it cleanly, at present. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:18 [email protected] parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: [email protected] @ 2024-05-15 19:18 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Josef Šimánek <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> Tom Lane: >> This is really what is missing for the ecosystem. A libpqparser for >> tools to use: Formatters, linters, query rewriters, simple syntax >> checkers... they are all missing access to postgres' own parser. > > To get to that, you'd need some kind of agreement on what the syntax > tree is. I doubt our existing implementation would be directly useful > to very many tools, and even if it is, do they want to track constant > version-to-version changes? Correct, on top of what the syntax tree currently has, one would probably need: - comments - locations (line number / character) for everything, including those of comments Otherwise it's impossible to print proper SQL again without losing information. And then on top of that, to be really really useful, you'd need to be able to parse partial statements, too, to support all kinds of "language server" applications. Tracking version-to-version changes is exactly the reason why it would be good to have that from upstream, imho. New syntax is added in (almost?) every release and everyone outside core trying to write their own parser and staying up2date with **all** the new syntax.. will eventually fail. Yes, there could be changes to the produced parse tree as well and you'd also need to adjust, for example, your SQL-printers. But it should be easier to stay up2date than right now. Best, Wolfgang ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:27 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 2 replies; 47+ messages in thread From: Tom Lane @ 2024-05-15 19:27 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Josef Šimánek <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> Robert Haas <[email protected]> writes: > On Wed, May 15, 2024 at 2:39 PM Tom Lane <[email protected]> wrote: >> The thing that was bothering me most about this is that I don't >> understand why that's a useful check. ... > But I don't understand the idea that the concept doesn't make sense. Sorry: "make sense" was a poorly chosen phrase there. What I was doubting, and continue to doubt, is that 100% checking of what you can check without catalog access and 0% checking of the rest is a behavior that end users will find especially useful. > I think it is perfectly reasonable to imagine a world in which the > initial parsing takes care of reporting everything that can be > determined by static analysis without knowing anything about catalog > contents, and parse analysis checks all the things that require > catalog access, and you can run the first set of checks and then > decide whether to proceed further. I think if I were designing a new > system from scratch, I'd definitely want to set it up that way, and I > think moving our existing system in that direction would probably let > us clean up a variety of warts along the way. Really, the only > argument I see against it is that getting from where we are to there > would be a gigantic amount of work for the value we'd derive. I'm less enthusiatic about this than you are. I think it would likely produce a slower and less maintainable system. Slower because we'd need more passes over the query: what parse analysis does today would have to be done in at least two separate steps. Less maintainable because knowledge about certain things would end up being more spread around the system. Taking your example of getting syntax checking to recognize invalid EXPLAIN keywords: right now there's just one piece of code that knows what those options are, but this'd require two. Also, "run the first set of checks and then decide whether to proceed further" seems like optimizing the system for broken queries over valid ones, which I don't think is an appropriate goal. Now, I don't say that there'd be *no* benefit to reorganizing the system that way. But it wouldn't be an unalloyed win, and so I share your bottom line that the costs would be out of proportion to the benefits. regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:32 David G. Johnston <[email protected]> parent: [email protected] 0 siblings, 2 replies; 47+ messages in thread From: David G. Johnston @ 2024-05-15 19:32 UTC (permalink / raw) To: [email protected]; +Cc: Tom Lane <[email protected]>; Josef Šimánek <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, May 15, 2024 at 12:18 PM <[email protected]> wrote: > Tom Lane: > >> This is really what is missing for the ecosystem. A libpqparser for > >> tools to use: Formatters, linters, query rewriters, simple syntax > >> checkers... they are all missing access to postgres' own parser. > > > > To get to that, you'd need some kind of agreement on what the syntax > > tree is. I doubt our existing implementation would be directly useful > > to very many tools, and even if it is, do they want to track constant > > version-to-version changes? > > Correct, on top of what the syntax tree currently has, one would > probably need: > - comments > - locations (line number / character) for everything, including those of > comments > > I'm with the original patch idea at this point. A utility that simply runs text through the parser, not parse analysis, and answers the question: "Were you able to parse this?" has both value and seems like something that can be patched into core in a couple of hundred lines, not thousands, as has already been demonstrated. Sure, other questions are valid and other goals exist in the ecosystem, but that doesn't diminish this one which is sufficiently justified for my +1 on the idea. Now, in my ideal world something like this could be made as an extension so that it can work on older versions and not have to be maintained by core. And likely grow more features over time. Is there anything fundamental about this that prevents it being implemented in an extension and, if so, what can we add to core in v18 to alleviate that limitation? David J. ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:33 Josef Šimánek <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 47+ messages in thread From: Josef Šimánek @ 2024-05-15 19:33 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> st 15. 5. 2024 v 21:28 odesílatel Tom Lane <[email protected]> napsal: > > Robert Haas <[email protected]> writes: > > On Wed, May 15, 2024 at 2:39 PM Tom Lane <[email protected]> wrote: > >> The thing that was bothering me most about this is that I don't > >> understand why that's a useful check. ... > > > But I don't understand the idea that the concept doesn't make sense. > > Sorry: "make sense" was a poorly chosen phrase there. What I was > doubting, and continue to doubt, is that 100% checking of what > you can check without catalog access and 0% checking of the rest > is a behavior that end users will find especially useful. But that's completely different feature which is not exclusive and shouldn't block this other feature to do only exactly as specified. > > I think it is perfectly reasonable to imagine a world in which the > > initial parsing takes care of reporting everything that can be > > determined by static analysis without knowing anything about catalog > > contents, and parse analysis checks all the things that require > > catalog access, and you can run the first set of checks and then > > decide whether to proceed further. I think if I were designing a new > > system from scratch, I'd definitely want to set it up that way, and I > > think moving our existing system in that direction would probably let > > us clean up a variety of warts along the way. Really, the only > > argument I see against it is that getting from where we are to there > > would be a gigantic amount of work for the value we'd derive. > > I'm less enthusiatic about this than you are. I think it would likely > produce a slower and less maintainable system. Slower because we'd > need more passes over the query: what parse analysis does today would > have to be done in at least two separate steps. Less maintainable > because knowledge about certain things would end up being more spread > around the system. Taking your example of getting syntax checking to > recognize invalid EXPLAIN keywords: right now there's just one piece > of code that knows what those options are, but this'd require two. > Also, "run the first set of checks and then decide whether to proceed > further" seems like optimizing the system for broken queries over > valid ones, which I don't think is an appropriate goal. > > Now, I don't say that there'd be *no* benefit to reorganizing the > system that way. But it wouldn't be an unalloyed win, and so I > share your bottom line that the costs would be out of proportion > to the benefits. > > regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:35 Josef Šimánek <[email protected]> parent: David G. Johnston <[email protected]> 1 sibling, 1 reply; 47+ messages in thread From: Josef Šimánek @ 2024-05-15 19:35 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: [email protected]; Tom Lane <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> st 15. 5. 2024 v 21:33 odesílatel David G. Johnston <[email protected]> napsal: > > On Wed, May 15, 2024 at 12:18 PM <[email protected]> wrote: >> >> Tom Lane: >> >> This is really what is missing for the ecosystem. A libpqparser for >> >> tools to use: Formatters, linters, query rewriters, simple syntax >> >> checkers... they are all missing access to postgres' own parser. >> > >> > To get to that, you'd need some kind of agreement on what the syntax >> > tree is. I doubt our existing implementation would be directly useful >> > to very many tools, and even if it is, do they want to track constant >> > version-to-version changes? >> >> Correct, on top of what the syntax tree currently has, one would >> probably need: >> - comments >> - locations (line number / character) for everything, including those of >> comments >> > > I'm with the original patch idea at this point. A utility that simply runs text through the parser, not parse analysis, and answers the question: "Were you able to parse this?" has both value and seems like something that can be patched into core in a couple of hundred lines, not thousands, as has already been demonstrated. > > Sure, other questions are valid and other goals exist in the ecosystem, but that doesn't diminish this one which is sufficiently justified for my +1 on the idea. > > Now, in my ideal world something like this could be made as an extension so that it can work on older versions and not have to be maintained by core. And likely grow more features over time. Is there anything fundamental about this that prevents it being implemented in an extension and, if so, what can we add to core in v18 to alleviate that limitation? Like extension providing additional binary? Or what kind of extension do you mean? One of the original ideas was to be able to do so (parse query) without running postgres itself. Could extension be useful without running postgres backend? > David J. > ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:43 David G. Johnston <[email protected]> parent: Josef Šimánek <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: David G. Johnston @ 2024-05-15 19:43 UTC (permalink / raw) To: Josef Šimánek <[email protected]>; +Cc: [email protected]; Tom Lane <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, May 15, 2024 at 12:35 PM Josef Šimánek <[email protected]> wrote: > st 15. 5. 2024 v 21:33 odesílatel David G. Johnston > <[email protected]> napsal: > > > Now, in my ideal world something like this could be made as an extension > so that it can work on older versions and not have to be maintained by > core. And likely grow more features over time. Is there anything > fundamental about this that prevents it being implemented in an extension > and, if so, what can we add to core in v18 to alleviate that limitation? > > Like extension providing additional binary? Or what kind of extension > do you mean? One of the original ideas was to be able to do so (parse > query) without running postgres itself. Could extension be useful > without running postgres backend? > > Pushing beyond my experience level here...but yes a separately installed binary (extension is being used conceptually here, this doesn't involve "create extension") that can inspect pg_config to find out where backend/postmaster library objects are installed and link to them. David J. ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 19:45 Tom Lane <[email protected]> parent: David G. Johnston <[email protected]> 1 sibling, 0 replies; 47+ messages in thread From: Tom Lane @ 2024-05-15 19:45 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: [email protected]; Josef Šimánek <[email protected]>; Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> "David G. Johnston" <[email protected]> writes: > Now, in my ideal world something like this could be made as an extension so > that it can work on older versions and not have to be maintained by core. > And likely grow more features over time. Is there anything fundamental > about this that prevents it being implemented in an extension and, if so, > what can we add to core in v18 to alleviate that limitation? It'd be pretty trivial to create a function that takes a string and runs it through raw_parser --- I've got such things laying about for microbenchmarking purposes, in fact. But the API that'd present for tools such as editors is enormously different from the proposed patch: there would need to be a running server and they'd need to be able to log into it, plus there are more minor concerns like having to wrap the string in valid quoting. Now on the plus side, once you'd bought into that environment, it'd be equally trivial to offer alternatives like "run raw parsing and parse analysis, but don't run the query". I continue to maintain that that's the set of checks you'd really want in a lot of use-cases. regards, tom lane ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-15 20:00 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 47+ messages in thread From: Robert Haas @ 2024-05-15 20:00 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Josef Šimánek <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, May 15, 2024 at 3:28 PM Tom Lane <[email protected]> wrote: > Sorry: "make sense" was a poorly chosen phrase there. What I was > doubting, and continue to doubt, is that 100% checking of what > you can check without catalog access and 0% checking of the rest > is a behavior that end users will find especially useful. You might be right, but my guess is that you're wrong. Syntax highlighting is very popular, and seems like a more sophisticated version of that same concept. I don't personally like it or use it myself, but I bet I'm hugely in the minority these days. And EDB certainly gets customer requests for syntax checking of various kinds; whether this particular kind would get more or less traction than other things is somewhat moot in view of the low likelihood of it actually happening. > I'm less enthusiatic about this than you are. I think it would likely > produce a slower and less maintainable system. Slower because we'd > need more passes over the query: what parse analysis does today would > have to be done in at least two separate steps. Less maintainable > because knowledge about certain things would end up being more spread > around the system. Taking your example of getting syntax checking to > recognize invalid EXPLAIN keywords: right now there's just one piece > of code that knows what those options are, but this'd require two. > Also, "run the first set of checks and then decide whether to proceed > further" seems like optimizing the system for broken queries over > valid ones, which I don't think is an appropriate goal. Well, we've talked before about other problems that stem from the fact that DDL doesn't have a clear separation between parse analysis and execution. I vaguely imagine that it would be valuable to clean that up for various reasons. But I haven't really thought it through, so I'm prepared to concede that it might have various downsides that aren't presently obvious to me. > Now, I don't say that there'd be *no* benefit to reorganizing the > system that way. But it wouldn't be an unalloyed win, and so I > share your bottom line that the costs would be out of proportion > to the benefits. I'm glad we agree on that much, and don't feel a compelling need to litigate the remaining differences between our positions, unless you really want to. I'm just telling you what I think, and I'm pleased that we think as similarly as we do, despite remaining differences. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-16 01:35 David G. Johnston <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 47+ messages in thread From: David G. Johnston @ 2024-05-16 01:35 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Josef Šimánek <[email protected]>; Tomas Vondra <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, May 15, 2024 at 1:00 PM Robert Haas <[email protected]> wrote: > On Wed, May 15, 2024 at 3:28 PM Tom Lane <[email protected]> wrote: > > Sorry: "make sense" was a poorly chosen phrase there. What I was > > doubting, and continue to doubt, is that 100% checking of what > > you can check without catalog access and 0% checking of the rest > > is a behavior that end users will find especially useful. > > You might be right, but my guess is that you're wrong. Syntax > highlighting is very popular, and seems like a more sophisticated > version of that same concept. The proposed seems distinctly less sophisticated though would be a starting point. I think the documentation for --syntax check would read something like this: postgres --syntax-check={filename | -} Performs a pass over the lexical structure of the script supplied in filename or, if - is specified, standard input, then exits. The exit code is zero if no errors were found, otherwise it is 1, and the errors, at full verbosity, are printed to standard error. This does not involve reading the configuration file and, by extension, will not detect errors that require knowledge of a database schema, including the system catalogs, to manifest. There will be no false positives, but due to the prior rule, false negatives must be factored into its usage. Thus this option is most useful as an initial triage point, quickly rejecting SQL code without requiring a running PostgreSQL service. Note: This is exposed as a convenient way to get access to the outcome of performing a raw parse within the specific version of the postgres binary being executed. The specific implementation of that parse is still non-public. Likewise, PostgreSQL doesn't itself have a use for a raw parse output beyond sending it to post-parse analysis. All of the catalog required checks, and potentially some that don't obviously need the catalogs, happen in this post-parse step; which the syntax checking API does not expose. In short, the API here doesn't include any guarantees regarding the specific errors one should expect to see output, only the no false positive test result of performing the first stage raw parse. David J. If in core I would still want to expose this as say a contrib module binary instead of hacking it into postgres. It would be our first server program entry there. ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-16 01:39 David G. Johnston <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: David G. Johnston @ 2024-05-16 01:39 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Josef Šimánek <[email protected]>; Tomas Vondra <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, May 15, 2024 at 6:35 PM David G. Johnston < [email protected]> wrote: > > If in core I would still want to expose this as say a contrib module > binary instead of hacking it into postgres. It would be our first server > program entry there. > > Sorry for self-reply but: Maybe name it "pg_script_check" with a, for now mandatory, "--syntax-only" option that enables this raw parse mode. Leaving room for executing this in an environment where there is, or it can launch, a running instance that then performs post-parse analysis as well. David J. ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-16 09:03 Laurenz Albe <[email protected]> parent: Tom Lane <[email protected]> 3 siblings, 0 replies; 47+ messages in thread From: Laurenz Albe @ 2024-05-16 09:03 UTC (permalink / raw) To: Tom Lane <[email protected]>; Josef Šimánek <[email protected]>; +Cc: Robert Haas <[email protected]>; Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, 2024-05-15 at 14:39 -0400, Tom Lane wrote: > The thing that was bothering me most about this is that I don't > understand why that's a useful check. If I meant to type > > UPDATE mytab SET mycol = 42; > > and instead I type > > UPDATEE mytab SET mycol = 42; > > your proposed feature would catch that; great. But if I type > > UPDATE mytabb SET mycol = 42; > > it won't. How does that make sense? It makes sense to me. I see a clear distinction between "this is a valid SQL statement" and "this is an SQL statement that will run on a specific database with certain objects in it". To me, "correct syntax" is the former. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 47+ messages in thread
* Re: [PATCH] Add --syntax to postgres for SQL syntax checking @ 2024-05-16 10:29 Peter Eisentraut <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 47+ messages in thread From: Peter Eisentraut @ 2024-05-16 10:29 UTC (permalink / raw) To: Robert Haas <[email protected]>; Josef Šimánek <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David G. Johnston <[email protected]>; Tom Lane <[email protected]>; Laurenz Albe <[email protected]>; PostgreSQL Hackers <[email protected]> On 15.05.24 21:05, Robert Haas wrote: > I don't think it's at all obvious that this belongs on the server side > rather than the client side. I think it could be done in either place, > or both. I just think we don't have the infrastructure to do it > cleanly, at present. I think if you're going to do a syntax-check-with-catalog-lookup mode, you need authentication and access control. The mode without catalog lookup doesn't need that. But it might be better to offer both modes through a similar interface (or at least plan ahead for that). ^ permalink raw reply [nested|flat] 47+ messages in thread
end of thread, other threads:[~2024-05-16 10:29 UTC | newest] Thread overview: 47+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-04-03 00:13 [PATCH v1] Cleanup/remove/update references to OID column... Justin Pryzby <[email protected]> 2019-04-03 00:13 [PATCH v2] Cleanup/remove/update references to OID column... Justin Pryzby <[email protected]> 2019-04-03 00:13 [PATCH v3] Cleanup/remove/update references to OID column... Justin Pryzby <[email protected]> 2019-04-03 00:13 [PATCH v3 12/12] Cleanup/remove/update references to OID column... Justin Pryzby <[email protected]> 2019-05-08 18:57 [PATCH v1] Cleanup/remove/update references to OID column... Justin Pryzby <[email protected]> 2022-11-23 07:52 drop postmaster symlink Peter Eisentraut <[email protected]> 2022-11-23 14:18 ` Re: drop postmaster symlink Joe Conway <[email protected]> 2022-11-23 14:28 ` Re: drop postmaster symlink Devrim Gündüz <[email protected]> 2022-11-23 15:07 ` Re: drop postmaster symlink Tom Lane <[email protected]> 2022-11-23 19:50 ` Re: drop postmaster symlink Andres Freund <[email protected]> 2022-11-23 20:10 ` Re: drop postmaster symlink Robert Haas <[email protected]> 2022-11-23 20:32 ` Re: drop postmaster symlink Joe Conway <[email protected]> 2022-11-24 00:15 ` Re: drop postmaster symlink Daniel Gustafsson <[email protected]> 2022-11-23 20:48 ` Re: drop postmaster symlink Tom Lane <[email protected]> 2022-11-23 21:08 ` Re: drop postmaster symlink Andres Freund <[email protected]> 2023-12-15 12:21 [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2023-12-15 13:09 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Laurenz Albe <[email protected]> 2023-12-15 13:14 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2023-12-15 13:19 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Pavel Stehule <[email protected]> 2023-12-15 14:50 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Tom Lane <[email protected]> 2023-12-15 15:05 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2023-12-15 15:16 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking David G. Johnston <[email protected]> 2023-12-15 15:20 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2023-12-15 15:31 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking David G. Johnston <[email protected]> 2023-12-15 15:38 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2024-02-25 22:24 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Tomas Vondra <[email protected]> 2024-05-15 17:42 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Robert Haas <[email protected]> 2024-05-15 18:12 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2024-05-15 18:39 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Tom Lane <[email protected]> 2024-05-15 18:49 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking [email protected] 2024-05-15 19:01 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Tom Lane <[email protected]> 2024-05-15 19:18 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking [email protected] 2024-05-15 19:32 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking David G. Johnston <[email protected]> 2024-05-15 19:35 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2024-05-15 19:43 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking David G. Johnston <[email protected]> 2024-05-15 19:45 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Tom Lane <[email protected]> 2024-05-15 19:00 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2024-05-15 19:03 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Robert Haas <[email protected]> 2024-05-15 19:27 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Tom Lane <[email protected]> 2024-05-15 19:33 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[email protected]> 2024-05-15 20:00 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Robert Haas <[email protected]> 2024-05-16 01:35 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking David G. Johnston <[email protected]> 2024-05-16 01:39 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking David G. Johnston <[email protected]> 2024-05-16 09:03 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Laurenz Albe <[email protected]> 2024-05-15 19:05 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Robert Haas <[email protected]> 2024-05-16 10:29 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Peter Eisentraut <[email protected]> 2024-05-15 18:13 ` Re: [PATCH] Add --syntax to postgres for SQL syntax checking Josef Šimánek <[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