Re: build remaining Flex files standalone
On Wed, Sep 14, 2022 at 6:10 AM Andres Freund wrote: > > On 2022-09-12 14:49:50 +0700, John Naylor wrote: > > CI builds fine. For v2 I only adjusted the commit message. I'll push > > in a couple days unless there are objections. > > Makes sense to me. Thanks for working on it! This is done. -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
On 2022-09-12 14:49:50 +0700, John Naylor wrote: > CI builds fine. For v2 I only adjusted the commit message. I'll push > in a couple days unless there are objections. Makes sense to me. Thanks for working on it!
Re: build remaining Flex files standalone
On Fri, Sep 9, 2022 at 12:18 PM John Naylor
wrote:
>
> It seems gramparse.h isn't installed now? In any case, here's a patch
> to move gramparse to the backend dir and stop symlinking/ installing
> gram.h.
Looking more closely at src/include/Makefile, this is incorrect -- all
files in SUBDIRS are copied over. So moving gramparse.h to the backend
will automatically not install it. The explicit install rule for
gram.h was for vpath builds.
CI builds fine. For v2 I only adjusted the commit message. I'll push
in a couple days unless there are objections.
--
John Naylor
EDB: http://www.enterprisedb.com
From dcfe0976e6118a2c385b4473ceab76a742a6f1ff Mon Sep 17 00:00:00 2001
From: John Naylor
Date: Fri, 9 Sep 2022 11:57:39 +0700
Subject: [PATCH v2] Move gramparse.h to src/backend/parser
This header is semi-private, being used only in files related to
raw parsing, so move to the backend directory where those files
live. This allows removal of makefile rules that symlink gram.h to
src/include/parser, since gramparse.h can now include gram.h from within
the same directory. While at it, stop installing gram.h.
Per suggestion from Andres Freund and Peter Eisentraut
Discussion: https://www.postgresql.org/message-id/20220904181759.px6uosll6zbxcum5%40awork3.anarazel.de
---
src/backend/Makefile| 7 +--
src/backend/parser/gram.y | 2 +-
src/{include => backend}/parser/gramparse.h | 4 ++--
src/backend/parser/parser.c | 2 +-
src/backend/parser/scan.l | 2 +-
src/include/Makefile| 4 ++--
src/include/parser/.gitignore | 1 -
src/tools/msvc/Install.pm | 4
src/tools/pginclude/cpluspluscheck | 1 -
src/tools/pginclude/headerscheck| 1 -
10 files changed, 8 insertions(+), 20 deletions(-)
rename src/{include => backend}/parser/gramparse.h (97%)
delete mode 100644 src/include/parser/.gitignore
diff --git a/src/backend/Makefile b/src/backend/Makefile
index d0d34821d5..181c217fae 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -153,12 +153,7 @@ submake-utils-headers:
.PHONY: generated-headers
-generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/storage/lwlocknames.h submake-catalog-headers submake-nodes-headers submake-utils-headers
-
-$(top_builddir)/src/include/parser/gram.h: parser/gram.h
- prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \
- cd '$(dir $@)' && rm -f $(notdir $@) && \
- $(LN_S) "$$prereqdir/$(notdir $<)" .
+generated-headers: $(top_builddir)/src/include/storage/lwlocknames.h submake-catalog-headers submake-nodes-headers submake-utils-headers
$(top_builddir)/src/include/storage/lwlocknames.h: storage/lmgr/lwlocknames.h
prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ea33784316..82f03fc9c9 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -55,9 +55,9 @@
#include "catalog/pg_trigger.h"
#include "commands/defrem.h"
#include "commands/trigger.h"
+#include "gramparse.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
-#include "parser/gramparse.h"
#include "parser/parser.h"
#include "storage/lmgr.h"
#include "utils/date.h"
diff --git a/src/include/parser/gramparse.h b/src/backend/parser/gramparse.h
similarity index 97%
rename from src/include/parser/gramparse.h
rename to src/backend/parser/gramparse.h
index 41b753a96c..c4726c618d 100644
--- a/src/include/parser/gramparse.h
+++ b/src/backend/parser/gramparse.h
@@ -11,7 +11,7 @@
* Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * src/include/parser/gramparse.h
+ * src/backend/parser/gramparse.h
*
*-
*/
@@ -26,7 +26,7 @@
* NB: include gram.h only AFTER including scanner.h, because scanner.h
* is what #defines YYLTYPE.
*/
-#include "parser/gram.h"
+#include "gram.h"
/*
* The YY_EXTRA data that a flex scanner allows us to pass around. Private
diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
index 50227cc098..ef85d3bb68 100644
--- a/src/backend/parser/parser.c
+++ b/src/backend/parser/parser.c
@@ -22,7 +22,7 @@
#include "postgres.h"
#include "mb/pg_wchar.h"
-#include "parser/gramparse.h"
+#include "gramparse.h"
#include "parser/parser.h"
#include "parser/scansup.h"
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 882e081aae..db8b0fe8eb 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -36,7 +36,7 @@
#include
#include "common/string.h"
-#include "parser/gramparse.h"
+#include "gramparse.h"
#include "parser/parser.h" /* only needed for GUC variables */
#include "parser/scansup.h"
#include "port/pg_bitutils.h"
diff --git a/src/include/Makefile
Re: build remaining Flex files standalone
On Wed, Sep 7, 2022 at 4:27 PM Peter Eisentraut
wrote:
>
> On 04.09.22 20:17, Andres Freund wrote:
> > I think, as a followup improvement, we should move gramparse.h to
> > src/backend/parser, and stop installing gram.h, gramparse.h. gramparse.h
> > already had this note:
> >
> > * NOTE: this file is only meant to be included in the core parsing files,
> > * i.e., parser.c, gram.y, and scan.l.
> > * Definitions that are needed outside the core parser should be in
> > parser.h.
> >
> > What do you think?
>
> I found in my notes:
>
> * maybe gram.h and gramparse.h should not be installed
>
> So, yeah. ;-)
It seems gramparse.h isn't installed now? In any case, here's a patch
to move gramparse to the backend dir and stop symlinking/ installing
gram.h. Confusingly, MSVC didn't seem to copy gram.h to src/include,
so I'm not yet sure how it still managed to build...
--
John Naylor
EDB: http://www.enterprisedb.com
From ff89180d7b0fe4c95a470a657ee465c5384d6ecc Mon Sep 17 00:00:00 2001
From: John Naylor
Date: Fri, 9 Sep 2022 11:57:39 +0700
Subject: [PATCH v1] Move gramparse.h to src/backend/parser
This allows removal of makefile rules that symlink gram.h to
src/include/parser. While at it, stop installing gram.h. This
seems unnecessary at present time.
Idea from Andres Freund and Peter Eisentraut
Discussion: https://www.postgresql.org/message-id/20220904181759.px6uosll6zbxcum5%40awork3.anarazel.de
---
src/backend/Makefile| 7 +--
src/backend/parser/gram.y | 2 +-
src/{include => backend}/parser/gramparse.h | 4 ++--
src/backend/parser/parser.c | 2 +-
src/backend/parser/scan.l | 2 +-
src/include/Makefile| 4 ++--
src/include/parser/.gitignore | 1 -
src/tools/msvc/Install.pm | 4
src/tools/pginclude/cpluspluscheck | 1 -
src/tools/pginclude/headerscheck| 1 -
10 files changed, 8 insertions(+), 20 deletions(-)
rename src/{include => backend}/parser/gramparse.h (97%)
delete mode 100644 src/include/parser/.gitignore
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 5c4772298d..42cc3bda1d 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -153,12 +153,7 @@ submake-utils-headers:
.PHONY: generated-headers
-generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/storage/lwlocknames.h submake-catalog-headers submake-nodes-headers submake-utils-headers
-
-$(top_builddir)/src/include/parser/gram.h: parser/gram.h
- prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \
- cd '$(dir $@)' && rm -f $(notdir $@) && \
- $(LN_S) "$$prereqdir/$(notdir $<)" .
+generated-headers: $(top_builddir)/src/include/storage/lwlocknames.h submake-catalog-headers submake-nodes-headers submake-utils-headers
$(top_builddir)/src/include/storage/lwlocknames.h: storage/lmgr/lwlocknames.h
prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0492ff9a66..668ad3fa8e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -55,9 +55,9 @@
#include "catalog/pg_trigger.h"
#include "commands/defrem.h"
#include "commands/trigger.h"
+#include "gramparse.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
-#include "parser/gramparse.h"
#include "parser/parser.h"
#include "storage/lmgr.h"
#include "utils/date.h"
diff --git a/src/include/parser/gramparse.h b/src/backend/parser/gramparse.h
similarity index 97%
rename from src/include/parser/gramparse.h
rename to src/backend/parser/gramparse.h
index 41b753a96c..c4726c618d 100644
--- a/src/include/parser/gramparse.h
+++ b/src/backend/parser/gramparse.h
@@ -11,7 +11,7 @@
* Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * src/include/parser/gramparse.h
+ * src/backend/parser/gramparse.h
*
*-
*/
@@ -26,7 +26,7 @@
* NB: include gram.h only AFTER including scanner.h, because scanner.h
* is what #defines YYLTYPE.
*/
-#include "parser/gram.h"
+#include "gram.h"
/*
* The YY_EXTRA data that a flex scanner allows us to pass around. Private
diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
index 50227cc098..ef85d3bb68 100644
--- a/src/backend/parser/parser.c
+++ b/src/backend/parser/parser.c
@@ -22,7 +22,7 @@
#include "postgres.h"
#include "mb/pg_wchar.h"
-#include "parser/gramparse.h"
+#include "gramparse.h"
#include "parser/parser.h"
#include "parser/scansup.h"
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 882e081aae..db8b0fe8eb 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -36,7 +36,7 @@
#include
#include "common/string.h"
-#include "parser/gramparse.h"
+#include "gramparse.h"
#include
Re: build remaining Flex files standalone
On 04.09.22 20:17, Andres Freund wrote: I think, as a followup improvement, we should move gramparse.h to src/backend/parser, and stop installing gram.h, gramparse.h. gramparse.h already had this note: * NOTE: this file is only meant to be included in the core parsing files, * i.e., parser.c, gram.y, and scan.l. * Definitions that are needed outside the core parser should be in parser.h. What do you think? I found in my notes: * maybe gram.h and gramparse.h should not be installed So, yeah. ;-)
Re: build remaining Flex files standalone
On Mon, Sep 5, 2022 at 1:18 AM Andres Freund wrote: > > Hi, > > On 2022-09-04 12:16:10 +0700, John Naylor wrote: > > Pushed 01 and 02 separately, then squashed and pushed the rest. > > Thanks a lot! It does look a good bit cleaner to me now. > > I think, as a followup improvement, we should move gramparse.h to > src/backend/parser, and stop installing gram.h, gramparse.h. gramparse.h > already had this note: > > * NOTE: this file is only meant to be included in the core parsing files, > * i.e., parser.c, gram.y, and scan.l. > * Definitions that are needed outside the core parser should be in parser.h. > > What do you think? +1 for the concept, but haven't looked at the details. -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
Hi, On 2022-09-04 12:16:10 +0700, John Naylor wrote: > Pushed 01 and 02 separately, then squashed and pushed the rest. Thanks a lot! It does look a good bit cleaner to me now. I think, as a followup improvement, we should move gramparse.h to src/backend/parser, and stop installing gram.h, gramparse.h. gramparse.h already had this note: * NOTE: this file is only meant to be included in the core parsing files, * i.e., parser.c, gram.y, and scan.l. * Definitions that are needed outside the core parser should be in parser.h. What do you think? I looked for projects including gramparse.h ([1], and found libpg-query, pgpool, slony1 and oracfe: - libpg-query, pgpool are partial copies of our code so will catch up when they sync up, - slony1's [2] is a configure check, one that long seems outdated, because it's grepping for standard_conforming strings, which was moved out in 6566e37e027 in 2009. - As far as I can tell oracfe's include in sqlscan.l is vistigial, it compiles without it. And the include in parse_keywords.c is just required because it needs to include parser/scanner.h. Greetings, Andres Freund [1] https://codesearch.debian.net/search?q=gramparse.h=1=1 [2] https://git.postgresql.org/gitweb/?p=slony1-engine.git;a=blob;f=config/acx_libpq.m4;h=7653357c0a731e36ec637df5ab378832d9279c19;hb=HEAD#l530
Re: build remaining Flex files standalone
On Sat, Sep 3, 2022 at 1:29 AM Andres Freund wrote: > > Subject: [PATCH v4 01/11] Preparatory refactoring for compiling guc-file.c > > standalone > > Subject: [PATCH v4 02/11] Move private declarations shared between guc.c and > > guc-file.l to new header > > Subject: [PATCH v4 03/11] Build guc-file.c standalone > > 01-03 are a bit more complicated, but still look not far off. There's a FIXME > about failing headercheck. Fixed by adding utils/guc.h to the new internal header, which now lives in the same directory as guc.c and guc-file.l, similar to how I did json path in the last patch. Also removed the bogus include from v4 to . Pushed 01 and 02 separately, then squashed and pushed the rest. -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
On Sat, Sep 3, 2022 at 1:29 AM Andres Freund wrote: > > Hi John, > > Are you planning to press ahead with these? I was waiting for feedback on the latest set, so tomorrow I'll see about the FIXME and remove the leftover bogus include. I was thinking of applying the guc-file patches separately and then squashing the rest since they're *mostly* mechanical: > > Subject: [PATCH v4 01/11] Preparatory refactoring for compiling guc-file.c > > standalone > > Subject: [PATCH v4 02/11] Move private declarations shared between guc.c and > > guc-file.l to new header > > Subject: [PATCH v4 03/11] Build guc-file.c standalone > > 01-03 are a bit more complicated, but still look not far off. There's a FIXME > about failing headercheck. -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
Hi John, Are you planning to press ahead with these? > Subject: [PATCH v4 01/11] Preparatory refactoring for compiling guc-file.c > standalone > Subject: [PATCH v4 02/11] Move private declarations shared between guc.c and > guc-file.l to new header > Subject: [PATCH v4 03/11] Build guc-file.c standalone 01-03 are a bit more complicated, but still look not far off. There's a FIXME about failing headercheck. > Subject: [PATCH v4 04/11] Build bootscanner.c standalone > Subject: [PATCH v4 05/11] Build repl_scanner.c standalone > Subject: [PATCH v4 06/11] Build syncrep_scanner.c standalone > Subject: [PATCH v4 07/11] Build specscanner.c standalone > Subject: [PATCH v4 08/11] Build exprscan.c standalone LGTM > Subject: [PATCH v4 09/11] Build cubescan.c standalone > > Pass scanbuflen as a parameter to yyparse rather than > resorting to a global variable. Nice. > Subject: [PATCH v4 10/11] Build segscan.c standalone > Subject: [PATCH v4 11/11] Build jsonpath_scan.c standalone LGTM. Greetings, Andres Freund
Re: build remaining Flex files standalone
I wrote > [v4] This piece is a leftover from the last version, and forgot to remove it, will fix: diff --git a/contrib/cube/cubeparse.y b/contrib/cube/cubeparse.y index 7577c4515c..e3b750b695 100644 --- a/contrib/cube/cubeparse.y +++ b/contrib/cube/cubeparse.y @@ -7,6 +7,7 @@ #include "postgres.h" #include "cubedata.h" +#include "cube_internal.h" #include "utils/float.h" -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
> > > > > index dbe7d4f742..0b373048b5 100644 > > > > > --- a/contrib/cube/cubedata.h > > > > > +++ b/contrib/cube/cubedata.h > > > > > @@ -67,3 +67,7 @@ extern void cube_scanner_finish(void); > > > > > > > > > > /* in cubeparse.y */ > > > > > extern int cube_yyparse(NDBOX **result); > > > > > + > > > > > +/* All grammar constructs return strings */ > > > > > +#define YYSTYPE char * > > > > > > > > Why does this need to be defined in a semi-public header? If we do this > > > > in > > > > multiple files we'll end up with the danger of macro redefinition > > > > warnings. > > For v4, I #defined YYSTYPE Sorry for the misfire. Continuing on, I #defined YYSTYPE in cubescan.l before #including cubeparse.h. I also added scanbuflen to the %parse-param to prevent resorting to a global variable. The rest of the patches are unchanged. -- John Naylor EDB: http://www.enterprisedb.com From 2b95401d925bed67b2cb1eb9e8cdb1f1dd3bcc8e Mon Sep 17 00:00:00 2001 From: John Naylor Date: Tue, 16 Aug 2022 12:01:41 +0700 Subject: [PATCH v4 02/11] Move private declarations shared between guc.c and guc-file.l to new header FIXME: fails headerscheck --- src/backend/utils/misc/guc-file.l | 1 + src/backend/utils/misc/guc.c | 1 + src/include/utils/guc.h | 10 -- src/include/utils/guc_internal.h | 24 4 files changed, 26 insertions(+), 10 deletions(-) create mode 100644 src/include/utils/guc_internal.h diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index b4fa09749b..843838b1df 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -18,6 +18,7 @@ #include "miscadmin.h" #include "storage/fd.h" #include "utils/guc.h" +#include "utils/guc_internal.h" /* diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 66ab3912a0..293834fc13 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -100,6 +100,7 @@ #include "utils/builtins.h" #include "utils/bytea.h" #include "utils/float.h" +#include "utils/guc_internal.h" #include "utils/guc_tables.h" #include "utils/memutils.h" #include "utils/pg_locale.h" diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index aae071cd82..45ae1b537f 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -442,16 +442,6 @@ extern void GUC_check_errcode(int sqlerrcode); pre_format_elog_string(errno, TEXTDOMAIN), \ GUC_check_errhint_string = format_elog_string -/* functions shared between guc.c and guc-file.l */ -extern int guc_name_compare(const char *namea, const char *nameb); -extern ConfigVariable *ProcessConfigFileInternal(GucContext context, - bool applySettings, int elevel); -extern void record_config_file_error(const char *errmsg, - const char *config_file, - int lineno, - ConfigVariable **head_p, - ConfigVariable **tail_p); - /* * The following functions are not in guc.c, but are declared here to avoid * having to include guc.h in some widely used headers that it really doesn't diff --git a/src/include/utils/guc_internal.h b/src/include/utils/guc_internal.h new file mode 100644 index 00..5d5db6bdce --- /dev/null +++ b/src/include/utils/guc_internal.h @@ -0,0 +1,24 @@ +/* + * guc_internals.h + * + * Declarations shared between backend/utils/misc/guc.c and + * backend/utils/misc/guc-file.l + * + * Copyright (c) 2000-2022, PostgreSQL Global Development Group + * + * src/include/utils/guc_internals.h + * + */ +#ifndef GUC_INTERNALS_H +#define GUC_INTERNALS_H + +extern int guc_name_compare(const char *namea, const char *nameb); +extern ConfigVariable *ProcessConfigFileInternal(GucContext context, + bool applySettings, int elevel); +extern void record_config_file_error(const char *errmsg, + const char *config_file, + int lineno, + ConfigVariable **head_p, + ConfigVariable **tail_p); + +#endif /* GUC_INTERNALS_H */ -- 2.36.1 From c066efea2193be8e7b21bb44c067383f34f37ec8 Mon Sep 17 00:00:00 2001 From: John Naylor Date: Tue, 16 Aug 2022 10:42:19 +0700 Subject: [PATCH v4 01/11] Preparatory refactoring for compiling guc-file.c standalone Mostly this involves moving ProcessConfigFileInternal() to guc.c and fixing the shared API to match. --- src/backend/utils/misc/guc-file.l | 360 +- src/backend/utils/misc/guc.c | 360 +- src/include/utils/guc.h | 9 + 3 files changed, 364 insertions(+), 365 deletions(-) diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index ce5633844c..b4fa09749b 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -48,12 +48,6 @@ static sigjmp_buf *GUC_flex_fatal_jmp; static
Re: build remaining Flex files standalone
On Wed, Aug 17, 2022 at 8:14 AM Andres Freund wrote: > > Hi, > > On 2022-08-16 17:41:43 +0700, John Naylor wrote: > > For v3, I addressed some comments and added .h files to the > > headerscheck exceptions. > > Thanks! > > > > /* > > * NB: include bootparse.h only AFTER including bootstrap.h, because > > bootstrap.h > > * includes node definitions needed for YYSTYPE. > > */ > > > > Future cleanup: I see this in headerscheck: > > > > # We can't make these Bison output files compilable standalone > > # without using "%code require", which old Bison versions lack. > > # parser/gram.h will be included by parser/gramparse.h anyway. > > > > That directive has been supported in Bison since 2.4.2. > > 2.4.2 is from 2010. So I think we could just start relying on it? > > > > > > +/* functions shared between guc.c and guc-file.l */ > > > > [...] > > > I think I prefer your suggestion of a guc_internal.h upthread. > > > > Started in 0002, but left open the headerscheck failure. > > > > Also, if such a thing is meant to be #include'd only by two generated > > files, maybe it should just live in the directory where they live, and > > not in the src/include dir? > > It's not something we've done for the backend afaics, but I don't see a reason > not to start at some point. > > > > > > From 7d4ecfcb3e91f3b45e94b9e64c7c40f1bbd22aa8 Mon Sep 17 00:00:00 2001 > > > > From: John Naylor > > > > Date: Fri, 12 Aug 2022 15:45:24 +0700 > > > > Subject: [PATCH v201 2/9] Build booscanner.c standalone > > > > > > > -# bootscanner is compiled as part of bootparse > > > > -bootparse.o: bootscanner.c > > > > +# See notes in src/backend/parser/Makefile about the following two > > > > rules > > > > +bootparse.h: bootparse.c > > > > + touch $@ > > > > + > > > > +bootparse.c: BISONFLAGS += -d > > > > + > > > > +# Force these dependencies to be known even without dependency info > > > > built: > > > > +bootparse.o bootscan.o: bootparse.h > > > > > > Wonder if we could / should wrap this is something common. It's somewhat > > > annoying to repeat this stuff everywhere. > > > > I haven't looked at the Meson effort recently, but if the build rule > > is less annoying there, I'm inclined to leave this as a wart until > > autotools are retired. > > The only complicating thing in the rules there is the dependencies from one .c > file to another .c file. > > > > > > diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h > > > > index dbe7d4f742..0b373048b5 100644 > > > > --- a/contrib/cube/cubedata.h > > > > +++ b/contrib/cube/cubedata.h > > > > @@ -67,3 +67,7 @@ extern void cube_scanner_finish(void); > > > > > > > > /* in cubeparse.y */ > > > > extern int cube_yyparse(NDBOX **result); > > > > + > > > > +/* All grammar constructs return strings */ > > > > +#define YYSTYPE char * > > > > > > Why does this need to be defined in a semi-public header? If we do this in > > > multiple files we'll end up with the danger of macro redefinition > > > warnings. For v4, I #defined YYSTYPE -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
On Wed, Aug 17, 2022 at 8:14 AM Andres Freund wrote: > > > > +/* functions shared between guc.c and guc-file.l */ > > > > [...] > > > I think I prefer your suggestion of a guc_internal.h upthread. > > > > Started in 0002, but left open the headerscheck failure. > > > > Also, if such a thing is meant to be #include'd only by two generated > > files, maybe it should just live in the directory where they live, and > > not in the src/include dir? > > It's not something we've done for the backend afaics, but I don't see a reason > not to start at some point. BTW, I forgot to mention I did this for the json path parser, which makes the makefile code simpler than what was there before 550b9d26f80fa30. AFAICS, we could also do the same for gramparse.h, which is internal to parser.c. If I'm not mistaken, the only reason we symlink gram.h to src/include/* is so that gramparse.h can include it. So keeping gramparse.h in the backend could allow removing some gram.h makefile incantations. > > > Why does this need to be defined in a semi-public header? If we do this in > > > multiple files we'll end up with the danger of macro redefinition > > > warnings. > > > > I tried to put all the Flex/Bison stuff in another *_internal header, > > but that breaks the build. Putting just this one symbol in a header is > > silly, but done that way for now. Maybe two copies of the symbol? > > The problem is that if it's in a header you can't include another header with > such a define. That's fine if it's a .h that's just intended to be included by > a limited set of files, but for something like a header for a datatype that > might need to be included to e.g. define a PL transform or a new operator or > ... This would be solved by the %code requires thing, right? I believe it would. -- John Naylor EDB: http://www.enterprisedb.com
Re: build remaining Flex files standalone
Andres Freund writes: > On 2022-08-16 17:41:43 +0700, John Naylor wrote: >> That directive has been supported in Bison since 2.4.2. > 2.4.2 is from 2010. So I think we could just start relying on it? Apple is still shipping 2.3. Is this worth enough to make Mac users install a non-default Bison? I seriously doubt it. I don't say that there won't be a reason that justifies that at some point, but letting headerscheck test autogenerated files seems of only microscopic benefit :-( regards, tom lane
Re: build remaining Flex files standalone
Hi, On 2022-08-16 17:41:43 +0700, John Naylor wrote: > For v3, I addressed some comments and added .h files to the > headerscheck exceptions. Thanks! > /* > * NB: include bootparse.h only AFTER including bootstrap.h, because > bootstrap.h > * includes node definitions needed for YYSTYPE. > */ > > Future cleanup: I see this in headerscheck: > > # We can't make these Bison output files compilable standalone > # without using "%code require", which old Bison versions lack. > # parser/gram.h will be included by parser/gramparse.h anyway. > > That directive has been supported in Bison since 2.4.2. 2.4.2 is from 2010. So I think we could just start relying on it? > > > +/* functions shared between guc.c and guc-file.l */ > > > [...] > > I think I prefer your suggestion of a guc_internal.h upthread. > > Started in 0002, but left open the headerscheck failure. > > Also, if such a thing is meant to be #include'd only by two generated > files, maybe it should just live in the directory where they live, and > not in the src/include dir? It's not something we've done for the backend afaics, but I don't see a reason not to start at some point. > > > From 7d4ecfcb3e91f3b45e94b9e64c7c40f1bbd22aa8 Mon Sep 17 00:00:00 2001 > > > From: John Naylor > > > Date: Fri, 12 Aug 2022 15:45:24 +0700 > > > Subject: [PATCH v201 2/9] Build booscanner.c standalone > > > > > -# bootscanner is compiled as part of bootparse > > > -bootparse.o: bootscanner.c > > > +# See notes in src/backend/parser/Makefile about the following two rules > > > +bootparse.h: bootparse.c > > > + touch $@ > > > + > > > +bootparse.c: BISONFLAGS += -d > > > + > > > +# Force these dependencies to be known even without dependency info > > > built: > > > +bootparse.o bootscan.o: bootparse.h > > > > Wonder if we could / should wrap this is something common. It's somewhat > > annoying to repeat this stuff everywhere. > > I haven't looked at the Meson effort recently, but if the build rule > is less annoying there, I'm inclined to leave this as a wart until > autotools are retired. The only complicating thing in the rules there is the dependencies from one .c file to another .c file. > > > diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h > > > index dbe7d4f742..0b373048b5 100644 > > > --- a/contrib/cube/cubedata.h > > > +++ b/contrib/cube/cubedata.h > > > @@ -67,3 +67,7 @@ extern void cube_scanner_finish(void); > > > > > > /* in cubeparse.y */ > > > extern int cube_yyparse(NDBOX **result); > > > + > > > +/* All grammar constructs return strings */ > > > +#define YYSTYPE char * > > > > Why does this need to be defined in a semi-public header? If we do this in > > multiple files we'll end up with the danger of macro redefinition warnings. > > I tried to put all the Flex/Bison stuff in another *_internal header, > but that breaks the build. Putting just this one symbol in a header is > silly, but done that way for now. Maybe two copies of the symbol? The problem is that if it's in a header you can't include another header with such a define. That's fine if it's a .h that's just intended to be included by a limited set of files, but for something like a header for a datatype that might need to be included to e.g. define a PL transform or a new operator or ... This would be solved by the %code requires thing, right? Greetings, Andres Freund
Re: build remaining Flex files standalone
For v3, I addressed some comments and added .h files to the
headerscheck exceptions.
On Tue, Aug 16, 2022 at 1:11 AM Andres Freund wrote:
> On 2022-08-13 15:39:06 +0700, John Naylor wrote:
> > Here are the rest. Most of it was pretty straightforward, with the
> > main exception of jsonpath_scan.c, which is not quite finished. That
> > one passes tests but still has one compiler warning. I'm unsure how
> > much of what is there already is really necessary or was cargo-culted
> > from elsewhere without explanation. For starters, I'm not sure why the
> > grammar has a forward declaration of "union YYSTYPE". It's noteworthy
> > that it used to compile standalone, but with a bit more stuff, and
> > that was reverted in 550b9d26f80fa30. I can hack on it some more later
> > but I ran out of steam today.
I've got it in half-way decent shape now, with an *internal.h header
and some cleanups.
> > - Include order seems to matter for the grammar's .h file. I didn't
> > test if that was the case every time, and after a few miscompiles just
> > always made it the last inclusion, but I'm wondering if we should keep
> > those inclusions outside %top{} and put it at the start of the next
> > %{} ?
>
> I think we have a few of those dependencies already, see e.g.
> /*
> * NB: include gram.h only AFTER including scanner.h, because scanner.h
> * is what #defines YYLTYPE.
> */
Went with something like this in all cases:
/*
* NB: include bootparse.h only AFTER including bootstrap.h, because bootstrap.h
* includes node definitions needed for YYSTYPE.
*/
Future cleanup: I see this in headerscheck:
# We can't make these Bison output files compilable standalone
# without using "%code require", which old Bison versions lack.
# parser/gram.h will be included by parser/gramparse.h anyway.
That directive has been supported in Bison since 2.4.2.
> > From d723ba14acf56fd432e9e263db937fcc13fc0355 Mon Sep 17 00:00:00 2001
> > From: John Naylor
> > Date: Thu, 11 Aug 2022 19:38:37 +0700
> > Subject: [PATCH v201 1/9] Build guc-file.c standalone
>
> Might be worth doing some of the moving around here separately from the
> parser/scanner specific bits.
Done in 0001/0003.
> > +/* functions shared between guc.c and guc-file.l */
> > [...]
> I think I prefer your suggestion of a guc_internal.h upthread.
Started in 0002, but left open the headerscheck failure.
Also, if such a thing is meant to be #include'd only by two generated
files, maybe it should just live in the directory where they live, and
not in the src/include dir?
> > From 7d4ecfcb3e91f3b45e94b9e64c7c40f1bbd22aa8 Mon Sep 17 00:00:00 2001
> > From: John Naylor
> > Date: Fri, 12 Aug 2022 15:45:24 +0700
> > Subject: [PATCH v201 2/9] Build booscanner.c standalone
>
> > -# bootscanner is compiled as part of bootparse
> > -bootparse.o: bootscanner.c
> > +# See notes in src/backend/parser/Makefile about the following two rules
> > +bootparse.h: bootparse.c
> > + touch $@
> > +
> > +bootparse.c: BISONFLAGS += -d
> > +
> > +# Force these dependencies to be known even without dependency info built:
> > +bootparse.o bootscan.o: bootparse.h
>
> Wonder if we could / should wrap this is something common. It's somewhat
> annoying to repeat this stuff everywhere.
I haven't looked at the Meson effort recently, but if the build rule
is less annoying there, I'm inclined to leave this as a wart until
autotools are retired.
> > diff --git a/src/test/isolation/specscanner.l
> > b/src/test/isolation/specscanner.l
> > index aa6e89268e..2dc292c21d 100644
> > --- a/src/test/isolation/specscanner.l
> > +++ b/src/test/isolation/specscanner.l
> > @@ -1,4 +1,4 @@
> > -%{
> > +%top{
> > /*-
> > *
> > * specscanner.l
> > @@ -9,7 +9,14 @@
> > *
> > *-
> > */
> > +#include "postgres_fe.h"
>
> Miniscule nitpick: I think we typically leave an empty line between header and
> first include.
In a small unscientific sample it seems like the opposite is true
actually, but I'll at least try to be consistent within the patch set.
> > diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h
> > index dbe7d4f742..0b373048b5 100644
> > --- a/contrib/cube/cubedata.h
> > +++ b/contrib/cube/cubedata.h
> > @@ -67,3 +67,7 @@ extern void cube_scanner_finish(void);
> >
> > /* in cubeparse.y */
> > extern int cube_yyparse(NDBOX **result);
> > +
> > +/* All grammar constructs return strings */
> > +#define YYSTYPE char *
>
> Why does this need to be defined in a semi-public header? If we do this in
> multiple files we'll end up with the danger of macro redefinition warnings.
I tried to put all the Flex/Bison stuff in another *_internal header,
but that breaks the build. Putting just this one symbol in a header is
silly, but done that way for now. Maybe two copies of the symbol?
Another future cleanup: "%define api.prefix {cube_yy}" etc would
Re: build remaining Flex files standalone
Hi,
Thanks for your work on this!
On 2022-08-13 15:39:06 +0700, John Naylor wrote:
> Here are the rest. Most of it was pretty straightforward, with the
> main exception of jsonpath_scan.c, which is not quite finished. That
> one passes tests but still has one compiler warning. I'm unsure how
> much of what is there already is really necessary or was cargo-culted
> from elsewhere without explanation. For starters, I'm not sure why the
> grammar has a forward declaration of "union YYSTYPE". It's noteworthy
> that it used to compile standalone, but with a bit more stuff, and
> that was reverted in 550b9d26f80fa30. I can hack on it some more later
> but I ran out of steam today.
I'm not sure either...
> Other questions thus far:
>
> - "BISONFLAGS += -d" is now in every make file with a .y file -- can
> we just force that everywhere?
Hm. Not sure it's worth it, extensions might use our BISON stuff...
> - Include order seems to matter for the grammar's .h file. I didn't
> test if that was the case every time, and after a few miscompiles just
> always made it the last inclusion, but I'm wondering if we should keep
> those inclusions outside %top{} and put it at the start of the next
> %{} ?
I think we have a few of those dependencies already, see e.g.
/*
* NB: include gram.h only AFTER including scanner.h, because scanner.h
* is what #defines YYLTYPE.
*/
> From d723ba14acf56fd432e9e263db937fcc13fc0355 Mon Sep 17 00:00:00 2001
> From: John Naylor
> Date: Thu, 11 Aug 2022 19:38:37 +0700
> Subject: [PATCH v201 1/9] Build guc-file.c standalone
Might be worth doing some of the moving around here separately from the
parser/scanner specific bits.
> +/* functions shared between guc.c and guc-file.l */
> +extern int guc_name_compare(const char *namea, const char *nameb);
> +extern ConfigVariable *ProcessConfigFileInternal(GucContext context,
> +
> bool applySettings, int elevel);
> +extern void record_config_file_error(const char *errmsg,
> + const
> char *config_file,
> + int
> lineno,
> +
> ConfigVariable **head_p,
> +
> ConfigVariable **tail_p);
>
> /*
> * The following functions are not in guc.c, but are declared here to avoid
> --
> 2.36.1
>
I think I prefer your suggestion of a guc_internal.h upthread.
> From 7d4ecfcb3e91f3b45e94b9e64c7c40f1bbd22aa8 Mon Sep 17 00:00:00 2001
> From: John Naylor
> Date: Fri, 12 Aug 2022 15:45:24 +0700
> Subject: [PATCH v201 2/9] Build booscanner.c standalone
> -# bootscanner is compiled as part of bootparse
> -bootparse.o: bootscanner.c
> +# See notes in src/backend/parser/Makefile about the following two rules
> +bootparse.h: bootparse.c
> + touch $@
> +
> +bootparse.c: BISONFLAGS += -d
> +
> +# Force these dependencies to be known even without dependency info built:
> +bootparse.o bootscan.o: bootparse.h
Wonder if we could / should wrap this is something common. It's somewhat
annoying to repeat this stuff everywhere.
> diff --git a/src/test/isolation/specscanner.l
> b/src/test/isolation/specscanner.l
> index aa6e89268e..2dc292c21d 100644
> --- a/src/test/isolation/specscanner.l
> +++ b/src/test/isolation/specscanner.l
> @@ -1,4 +1,4 @@
> -%{
> +%top{
> /*-
> *
> * specscanner.l
> @@ -9,7 +9,14 @@
> *
> *-
> */
> +#include "postgres_fe.h"
Miniscule nitpick: I think we typically leave an empty line between header and
first include.
> diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h
> index dbe7d4f742..0b373048b5 100644
> --- a/contrib/cube/cubedata.h
> +++ b/contrib/cube/cubedata.h
> @@ -67,3 +67,7 @@ extern void cube_scanner_finish(void);
>
> /* in cubeparse.y */
> extern int cube_yyparse(NDBOX **result);
> +
> +/* All grammar constructs return strings */
> +#define YYSTYPE char *
Why does this need to be defined in a semi-public header? If we do this in
multiple files we'll end up with the danger of macro redefinition warnings.
> +extern int scanbuflen;
The code around scanbuflen seems pretty darn grotty. Allocating enough memory
for the entire list by allocating the entire string size... I don't know
anything about contrib/cube, but isn't that in effect O(inputlen^2) memory?
Greetings,
Andres Freund
Re: build remaining Flex files standalone
Here are the rest. Most of it was pretty straightforward, with the
main exception of jsonpath_scan.c, which is not quite finished. That
one passes tests but still has one compiler warning. I'm unsure how
much of what is there already is really necessary or was cargo-culted
from elsewhere without explanation. For starters, I'm not sure why the
grammar has a forward declaration of "union YYSTYPE". It's noteworthy
that it used to compile standalone, but with a bit more stuff, and
that was reverted in 550b9d26f80fa30. I can hack on it some more later
but I ran out of steam today.
Other questions thus far:
- "BISONFLAGS += -d" is now in every make file with a .y file -- can
we just force that everywhere?
- Include order seems to matter for the grammar's .h file. I didn't
test if that was the case every time, and after a few miscompiles just
always made it the last inclusion, but I'm wondering if we should keep
those inclusions outside %top{} and put it at the start of the next
%{} ?
- contrib/cubeparse.y now has a global variable -- not terrific, but I
wanted to get something working first.
- I'm actually okay with guc-file.c now, but I'll still welcome
comments on that.
--
John Naylor
EDB: http://www.enterprisedb.com
From da2b610b8608e6759f5ed9cc32b487ea8e750ce4 Mon Sep 17 00:00:00 2001
From: John Naylor
Date: Fri, 12 Aug 2022 17:09:45 +0700
Subject: [PATCH v201 3/9] Build repl_scanner.c standalone
---
src/backend/replication/Makefile | 11 +--
src/backend/replication/repl_gram.y| 2 --
src/backend/replication/repl_scanner.l | 27 +++---
3 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/src/backend/replication/Makefile b/src/backend/replication/Makefile
index 2bffac58c0..bc8170418f 100644
--- a/src/backend/replication/Makefile
+++ b/src/backend/replication/Makefile
@@ -16,6 +16,7 @@ override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
OBJS = \
repl_gram.o \
+ repl_scanner.o \
slot.o \
slotfuncs.o \
syncrep.o \
@@ -28,8 +29,14 @@ SUBDIRS = logical
include $(top_srcdir)/src/backend/common.mk
-# repl_scanner is compiled as part of repl_gram
-repl_gram.o: repl_scanner.c
+# See notes in src/backend/parser/Makefile about the following two rules
+repl_gram.h: repl_gram.c
+ touch $@
+
+repl_gram.c: BISONFLAGS += -d
+
+# Force these dependencies to be known even without dependency info built:
+repl_gram.o repl_scanner.o: repl_gram.h
# syncrep_scanner is compiled as part of syncrep_gram
syncrep_gram.o: syncrep_scanner.c
diff --git a/src/backend/replication/repl_gram.y b/src/backend/replication/repl_gram.y
index 4cf087e602..b343f108d3 100644
--- a/src/backend/replication/repl_gram.y
+++ b/src/backend/replication/repl_gram.y
@@ -416,5 +416,3 @@ ident_or_keyword:
;
%%
-
-#include "repl_scanner.c"
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index 586f0d3a5c..95a933c9a8 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -1,4 +1,4 @@
-%{
+%top{
/*-
*
* repl_scanner.l
@@ -17,7 +17,12 @@
#include "utils/builtins.h"
#include "parser/scansup.h"
+#include "replication/walsender_private.h"
+
+#include "repl_gram.h"
+}
+%{
/* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */
#undef fprintf
#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg)
@@ -130,7 +135,7 @@ WAIT{ return K_WAIT; }
{space}+ { /* do nothing */ }
{digit}+ {
- yylval.uintval = strtoul(yytext, NULL, 10);
+ replication_yylval.uintval = strtoul(yytext, NULL, 10);
return UCONST;
}
@@ -138,8 +143,8 @@ WAIT{ return K_WAIT; }
uint32 hi,
lo;
if (sscanf(yytext, "%X/%X", , ) != 2)
- yyerror("invalid streaming start location");
- yylval.recptr = ((uint64) hi) << 32 | lo;
+ replication_yyerror("invalid streaming start location");
+ replication_yylval.recptr = ((uint64) hi) << 32 | lo;
return RECPTR;
}
@@ -151,7 +156,7 @@ WAIT{ return K_WAIT; }
{quotestop} {
yyless(1);
BEGIN(INITIAL);
- yylval.str = litbufdup();
+ replication_yylval.str = litbufdup();
return SCONST;
}
@@ -173,9 +178,9 @@ WAIT{ return K_WAIT; }
yyless(1);
BEGIN(INITIAL);
- yylval.str = litbufdup();
- len = strlen(yylval.str);
- truncate_identifier(yylval.str, len, true);
+ replication_yylval.str = litbufdup();
+ len = strlen(replication_yylval.str);
+ truncate_identifier(replication_yylval.str, len, true);
return IDENT;
}
@@ -186,7 +191,7 @@ WAIT{ return K_WAIT; }
{identifier} {
int len = strlen(yytext);
- yylval.str = downcase_truncate_identifier(yytext, len, true);
+ replication_yylval.str = downcase_truncate_identifier(yytext, len, true);
return IDENT;
}
@@ -195,7 +200,7 @@ WAIT{ return K_WAIT; }
