Copilot commented on code in PR #2286:
URL: https://github.com/apache/age/pull/2286#discussion_r2669344264
##########
Makefile:
##########
@@ -150,8 +157,12 @@ src/backend/parser/cypher_gram.c: BISONFLAGS +=
--defines=src/include/parser/cyp
src/backend/parser/cypher_parser.o: src/backend/parser/cypher_gram.c
src/backend/parser/cypher_keywords.o: src/backend/parser/cypher_gram.c
-$(age_sql):
+# Strip PASSEDBYVALUE on 32-bit (SIZEOF_DATUM=4) for graphid pass-by-reference
+$(age_sql): $(SQLS)
@cat $(SQLS) > $@
+ifeq ($(SIZEOF_DATUM),4)
+ @sed 's/^[[:space:]]*PASSEDBYVALUE,$$/ -- PASSEDBYVALUE removed for
32-bit/' $@ > [email protected] && mv [email protected] $@
Review Comment:
The sed replacement pattern only targets lines with leading whitespace
followed by `PASSEDBYVALUE,`. However, if the SQL formatting changes in the
future (e.g., `PASSEDBYVALUE` appears without leading spaces or trailing comma
on the same line), the replacement will silently fail. Consider making the
pattern more robust or adding validation to ensure the replacement succeeded.
For example, you could check that the replacement actually occurred by using
sed's return value or grep to verify the comment was added.
```suggestion
@sed 's/^[[:space:]]*PASSEDBYVALUE[[:space:]]*,\?[[:space:]]*$$/ --
PASSEDBYVALUE removed for 32-bit/' $@ > [email protected] && mv [email protected] $@ && grep -q
'PASSEDBYVALUE removed for 32-bit' $@ || { echo "PASSEDBYVALUE marker not found
or not replaced in $@"; exit 1; }
```
##########
Makefile:
##########
@@ -138,6 +138,13 @@ PG_CONFIG ?= pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
+# 32-bit platform support: detect SIZEOF_DATUM (override with make
SIZEOF_DATUM=4)
+SIZEOF_DATUM ?= $(shell printf '%s\n%s\n' '\#include "pg_config.h"'
'SIZEOF_VOID_P' | \
+ $(CC) -I$(shell $(PG_CONFIG) --includedir-server) -E -x c - 2>/dev/null |
grep -E '^[0-9]+$$' | tail -1)
+ifeq ($(SIZEOF_DATUM),)
+ SIZEOF_DATUM := 8
Review Comment:
The SIZEOF_DATUM detection relies on the C preprocessor successfully
evaluating SIZEOF_VOID_P from pg_config.h. However, if the compilation fails or
the preprocessor encounters an error, stderr is redirected to /dev/null and the
detection silently falls back to 8. This could mask configuration issues where
PostgreSQL headers are not properly installed or the compiler is misconfigured.
Consider logging a warning or checking if pg_config.h exists before attempting
the detection, or verify that the detected value is sane (4 or 8) rather than
silently using a default.
```suggestion
# Only attempt auto-detection if SIZEOF_DATUM was not provided on the
command line.
ifeq ($(origin SIZEOF_DATUM), undefined)
SIZEOF_DATUM := $(shell printf '%s\n%s\n' '\#include "pg_config.h"'
'SIZEOF_VOID_P' | \
$(CC) -I$(shell $(PG_CONFIG) --includedir-server) -E -x c -
2>/dev/null | grep -E '^[0-9]+$$' | tail -1)
ifeq ($(SIZEOF_DATUM),)
$(warning Unable to detect SIZEOF_DATUM from pg_config.h; defaulting to
8. Check that PostgreSQL server headers are installed and that pg_config and
the compiler are correctly configured.)
SIZEOF_DATUM := 8
endif
ifeq ($(filter 4 8,$(SIZEOF_DATUM)),)
$(warning Detected unexpected SIZEOF_DATUM '$(SIZEOF_DATUM)'; expected 4
or 8. Defaulting to 8.)
SIZEOF_DATUM := 8
endif
```
##########
Makefile:
##########
@@ -150,8 +157,12 @@ src/backend/parser/cypher_gram.c: BISONFLAGS +=
--defines=src/include/parser/cyp
src/backend/parser/cypher_parser.o: src/backend/parser/cypher_gram.c
src/backend/parser/cypher_keywords.o: src/backend/parser/cypher_gram.c
-$(age_sql):
+# Strip PASSEDBYVALUE on 32-bit (SIZEOF_DATUM=4) for graphid pass-by-reference
+$(age_sql): $(SQLS)
@cat $(SQLS) > $@
+ifeq ($(SIZEOF_DATUM),4)
+ @sed 's/^[[:space:]]*PASSEDBYVALUE,$$/ -- PASSEDBYVALUE removed for
32-bit/' $@ > [email protected] && mv [email protected] $@
Review Comment:
The sed command replaces PASSEDBYVALUE with a SQL comment, but this creates
a semantic change in the generated SQL file that is only visible at build time.
When debugging issues on 32-bit platforms, developers might look at the source
SQL files and see PASSEDBYVALUE present, but the actual installed SQL has it
commented out. This discrepancy could cause confusion. Consider adding a
comment in the source SQL file (sql/age_main.sql) near the PASSEDBYVALUE line
to indicate it will be removed on 32-bit platforms, or use a more explicit
marker in the source that gets processed by sed.
```suggestion
@sed 's/^[[:space:]]*PASSEDBYVALUE,$$/ -- PASSEDBYVALUE disabled on
32-bit (replaced at build time; see Makefile SIZEOF_DATUM rule)/' $@ > [email protected]
&& mv [email protected] $@
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
