Copilot commented on code in PR #2286:
URL: https://github.com/apache/age/pull/2286#discussion_r2672920597
##########
Makefile:
##########
@@ -150,8 +165,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[[:space:]]*,\?[[:space:]]*$$/ --
PASSEDBYVALUE disabled on 32-bit (replaced at build time; see Makefile
SIZEOF_DATUM rule)/' $@ > [email protected] && mv [email protected] $@ && grep -q 'PASSEDBYVALUE
disabled on 32-bit' $@ || { echo "PASSEDBYVALUE marker not found or not
replaced in $@"; exit 1; }
Review Comment:
The sed replacement pattern uses a complex regular expression that may not
be portable across all sed implementations. Specifically, the character class
[[:space:]] is a POSIX extension that may not work with all versions of sed
(though it's widely supported).
Additionally, the pattern matches PASSEDBYVALUE with optional trailing comma
and whitespace on its own line. This is fragile because:
1. It won't match if PASSEDBYVALUE appears inline with other content
2. Minor formatting changes in the SQL source could break this pattern
3. The replacement creates a very long comment line that could exceed line
length limits
Consider a more targeted approach, such as:
- Using a script (perl/python) for more robust text processing
- Matching based on more context (e.g., within the CREATE TYPE graphid block)
- Using multiple simpler patterns instead of one complex pattern
```suggestion
@sed -e 's/^[ \t]*PASSEDBYVALUE[ \t]*,[ \t]*$/ -- PASSEDBYVALUE
disabled on 32-bit; see Makefile SIZEOF_DATUM/' -e 's/^[ \t]*PASSEDBYVALUE[
\t]*$/ -- PASSEDBYVALUE disabled on 32-bit; see Makefile SIZEOF_DATUM/' $@ >
[email protected] && mv [email protected] $@ && grep -q 'PASSEDBYVALUE disabled on 32-bit' $@ || {
echo "PASSEDBYVALUE marker not found or not replaced in $@"; exit 1; }
```
##########
Makefile:
##########
@@ -138,6 +138,21 @@ PG_CONFIG ?= pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
+# 32-bit platform support: detect SIZEOF_DATUM (override with make
SIZEOF_DATUM=4)
+# 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
+endif
+
Review Comment:
The PR description states "Add conditional compilation in graphid.h" to use
pass-by-value on 64-bit and pass-by-reference on 32-bit systems. However, the
actual code changes do not include any modifications to graphid.h or any other
C header/source files.
The macros in src/include/utils/graphid.h still unconditionally use:
- DATUM_GET_GRAPHID(d) as DatumGetInt64(d)
- GRAPHID_GET_DATUM(x) as Int64GetDatum(x)
These macros need to be conditionally compiled based on SIZEOF_DATUM to
handle pass-by-reference on 32-bit platforms. Without these changes, the build
will succeed but the code will fail at runtime on 32-bit systems because it's
still attempting to pass 8-byte values by value in a 4-byte Datum.
The C code must include conditional compilation logic similar to:
- On 64-bit: use DatumGetInt64/Int64GetDatum (pass-by-value)
- On 32-bit: use DatumGetPointer/PointerGetDatum with proper dereferencing
(pass-by-reference)
This is a critical missing piece that makes the current implementation
incomplete.
--
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]
