Conversation
The gen-typedefs.sh script accumulated duplicates on each run because it only appended to typedefs.list. Fixed by: - Uncommenting curl to fetch fresh PostgreSQL typedefs (overwrites file) - Adding sort/uniq to remove any internal duplicates - Cleaning up typedefs.list Sorting allow us to stabilise the content in case a struct or enum has been displaced. This ensures repeated script runs produce consistent, deduplicated output.
Automatically validates code formatting on PRs to main branch using pgindent. Builds pg_bsd_indent from PostgreSQL source, runs gen-typedefs.sh, and fails if formatting issues are detected.
Better name reflects the script's actual purpose: running pgindent on all source files. Updated README and GitHub workflow accordingly.
📝 WalkthroughWalkthroughAdds a GitHub Actions "pgindent check" workflow and pgindent automation; introduces named typedefs; updates Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/pgindent-check.yml:
- Around line 47-53: The CI step named "Check for formatting changes" currently
echoes "Please run gen-typedefs.sh locally" when git diff shows changes; update
that message to reference the correct script name `run-pgindent.sh` so
developers run the right formatter. Locate the block containing the name "Check
for formatting changes" and the `if ! git diff --quiet; then` check and replace
the echo text to "Please run run-pgindent.sh locally" (or include both commands
if applicable) to avoid confusion.
- Around line 30-39: Remove the dead PATH entry and pin the cloned PostgreSQL
ref: stop appending "$GITHUB_WORKSPACE/pg-install/bin" to GITHUB_PATH (it's
never created), and either change that echo to point to the actual install
prefix (/tmp/pg-build/bin) *or* run `make install` into /tmp/pg-build so the
directory exists; additionally replace the git clone --branch master with a
specific stable tag (for example REL_17_STABLE) to ensure reproducible builds
and stable pg_bsd_indent behavior.
In `@include/spock_conflict.h`:
- Line 85: The function declaration ending with "RepOriginId * local_origin,
TimestampTz *local_ts" uses incorrect spacing for the pointer parameter; change
it to "RepOriginId *local_origin" to match pgindent standard and the typedefs
list in src/spock_conflict.c, and update any other matching declarations in
include/spock_conflict.h to remove the space between '*' and the parameter name
(e.g., for local_origin) so the header matches the .c file style.
- Around line 79-81: Duplicate extern declarations of spock_conflict_resolver,
spock_conflict_log_level, and spock_save_resolutions exist; remove the redundant
declarations and keep the original ones. Locate the repeated extern declarations
for the symbols spock_conflict_resolver, spock_conflict_log_level, and
spock_save_resolutions (the duplicates around lines 79–81) and delete them so
only the original declarations (the ones around lines 42–44) remain.
In `@src/spock_conflict.c`:
- Line 182: The pgindent run is treating "RepOriginId" as a non-typedef
(producing pointer spacing "RepOriginId * local_origin"); add RepOriginId to the
typedefs list used by pgindent so it is recognized as a core typedef. Modify
utils/pgindent/run-pgindent.sh to include RepOriginId when fetching/combining
core typedefs from the buildfarm (or add it explicitly to the extraction list),
ensuring the combined typedefs file used by pgindent contains "RepOriginId" so
functions like the one declaring RepOriginId * local_origin are formatted
correctly.
In `@utils/pgindent/README.md`:
- Around line 10-12: The fenced code block containing "make install
prefix=/usr/local" is missing a language specifier (MD040); update that fenced
block by adding a shell language identifier (e.g., ```sh or ```bash) immediately
after the opening triple backticks so the block becomes a shell code block and
satisfies markdownlint.
In `@utils/pgindent/run-pgindent.sh`:
- Around line 1-16: Add strict error handling and make the remote fetch
fail-safe: at script start enable safe-fail flags (e.g., set -euo pipefail or at
minimum set -e) so any failing command (grep/awk/sort/xargs) aborts; change the
curl invocation to use fail-and-quiet flags and write to the file atomically
(e.g., curl -fsS -o typedefs.list <URL>) and immediately check its exit status
and/or that typedefs.list is non-empty and exit with a clear error if the fetch
failed; ensure the pipeline commands (grep/awk/sort/xargs) run under the same
failure semantics so a failed grep/awk or xargs won’t silently produce a bad
typedefs.list.
🧹 Nitpick comments (6)
src/spock_functions.c (3)
2236-2264: Consider usingcastNode()for consistency with the rest of the switch.Other cases in this switch (e.g.,
AlterOwnerStmt,RenameStmt,CreateTableAsStmt) usecastNode(), which provides a runtime assertion in debug builds. The C-style cast(ClusterStmt *) stmton Line 2238 works but is inconsistent. Same applies to(ReindexStmt *)on Line 2292.♻️ Suggested change
- ClusterStmt *cstmt = (ClusterStmt *) stmt; + ClusterStmt *cstmt = castNode(ClusterStmt, stmt);
2290-2307: SamecastNodeinconsistency as above.♻️ Suggested change
- ReindexStmt *rstmt = (ReindexStmt *) stmt; + ReindexStmt *rstmt = castNode(ReindexStmt, stmt);
3499-3500:valuesarray is not zero-initialized whilenullsis.This is fine here since every index in
_GP_LAST_is explicitly assigned in the code below (either invalues[...]or guarded bynulls[...] = true), andtuplestore_putvaluesignores values for null columns. Just flagging for awareness — if a newGP_*enum member is added later without a corresponding assignment,valueswould contain garbage for that column.src/spock_autoddl.c (1)
225-232: Blank line betweenifand its body harms readability.The blank line at Line 227 between
if (targetrel == NULL)and the comment+returnmakes it visually ambiguous whether thereturnis controlled by theif. Without braces, a reader may think theifbody is empty and thereturnis unconditional.Consider wrapping in braces to make the intent clear:
Suggested fix
- targetrel = table_openrv_extended(relation, AccessShareLock, missing_ok); - if (targetrel == NULL) - - /* - * If relation doesn't exist - quietly exit. It is assumed that - * the core already produced an INFO message. - */ - return; + targetrel = table_openrv_extended(relation, AccessShareLock, missing_ok); + if (targetrel == NULL) + { + /* + * If relation doesn't exist - quietly exit. It is assumed that + * the core already produced an INFO message. + */ + return; + }src/spock_group.c (1)
423-429: Same blank-line-before-body pattern as inspock_autoddl.c.Line 424 has a blank line between
elseand its controlled statement (theelog(ERROR, ...)), with a comment block in between. This is syntactically valid but visually suggests theelsebody is empty. Consider adding braces for clarity, consistent with the suggestion inspock_autoddl.c..github/workflows/pgindent-check.yml (1)
31-39: Quote shell variables to prevent globbing and word splitting.Per static analysis (shellcheck SC2086),
$GITHUB_WORKSPACEand$GITHUB_PATHshould be double-quoted on Lines 37–39.Suggested fix
- echo "$GITHUB_WORKSPACE/pg-install/bin" >> $GITHUB_PATH - echo "/tmp/postgresql/src/tools/pgindent" >> $GITHUB_PATH - echo "/tmp/postgresql/src/tools/pg_bsd_indent" >> $GITHUB_PATH + echo "$GITHUB_WORKSPACE/pg-install/bin" >> "$GITHUB_PATH" + echo "/tmp/postgresql/src/tools/pgindent" >> "$GITHUB_PATH" + echo "/tmp/postgresql/src/tools/pg_bsd_indent" >> "$GITHUB_PATH"
include/spock_conflict.h
Outdated
| extern bool get_tuple_origin(SpockRelation *rel, HeapTuple local_tuple, | ||
| ItemPointer tid, TransactionId *xmin, | ||
| RepOriginId *local_origin, TimestampTz *local_ts); | ||
| RepOriginId * local_origin, TimestampTz *local_ts); |
There was a problem hiding this comment.
Same RepOriginId * local_origin pointer spacing issue as in the .c file.
This should match the pgindent-standard RepOriginId *local_origin (no space between * and parameter name). See my comment on src/spock_conflict.c line 182 regarding the typedefs list.
🤖 Prompt for AI Agents
In `@include/spock_conflict.h` at line 85, The function declaration ending with
"RepOriginId * local_origin, TimestampTz *local_ts" uses incorrect spacing for
the pointer parameter; change it to "RepOriginId *local_origin" to match
pgindent standard and the typedefs list in src/spock_conflict.c, and update any
other matching declarations in include/spock_conflict.h to remove the space
between '*' and the parameter name (e.g., for local_origin) so the header
matches the .c file style.
src/spock_conflict.c
Outdated
| get_tuple_origin(SpockRelation *rel, HeapTuple local_tuple, ItemPointer tid, | ||
| TransactionId *xmin, | ||
| RepOriginId *local_origin, TimestampTz *local_ts) | ||
| RepOriginId * local_origin, TimestampTz *local_ts) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual line 182 in src/spock_conflict.c
head -185 src/spock_conflict.c | tail -10Repository: pgEdge/spock
Length of output: 372
🏁 Script executed:
# Search for typedefs files in pgindent utilities
find . -name "*typedefs*" -o -name "run-pgindent*" | head -20Repository: pgEdge/spock
Length of output: 119
🏁 Script executed:
# Check if RepOriginId exists in the codebase and see its definition
rg "typedef.*RepOriginId" --type cRepository: pgEdge/spock
Length of output: 38
🏁 Script executed:
# Check if RepOriginId is in the typedefs list
grep -n "RepOriginId" ./utils/pgindent/typedefs.listRepository: pgEdge/spock
Length of output: 38
🏁 Script executed:
# Read the run-pgindent.sh script to understand how it works
cat ./utils/pgindent/run-pgindent.shRepository: pgEdge/spock
Length of output: 830
🏁 Script executed:
# Search for RepOriginId definition in the codebase (broader search)
rg "RepOriginId" -B 2 -A 2 | head -50Repository: pgEdge/spock
Length of output: 2354
🏁 Script executed:
# Search for RepOriginId definition in header files
rg "typedef.*RepOriginId|#define.*RepOriginId" -t c -t hRepository: pgEdge/spock
Length of output: 38
🏁 Script executed:
# Search for RepOriginId in include files
find . -name "*.h" -type f -exec grep -l "RepOriginId" {} \;Repository: pgEdge/spock
Length of output: 203
🏁 Script executed:
# Check if RepOriginId comes from PostgreSQL server includes
rg "RepOriginId" -t h | grep -i include | head -10Repository: pgEdge/spock
Length of output: 751
🏁 Script executed:
# Search for includes of PostgreSQL replication headers
rg "#include" -t c -t h | grep -i "origin\|replication" | head -20Repository: pgEdge/spock
Length of output: 1221
🏁 Script executed:
# Check if there are any comments or notes about RepOriginId in the spock code
rg "RepOriginId" -B 3 -A 1 | grep -i "from pg\|postgresql\|core\|origin" | head -20Repository: pgEdge/spock
Length of output: 1272
🏁 Script executed:
# Look at the actual include directives in spock_conflict.h
head -50 ./include/spock_conflict.hRepository: pgEdge/spock
Length of output: 1372
🏁 Script executed:
# Verify the actual current typedefs.list content to confirm RepOriginId absence
wc -l ./utils/pgindent/typedefs.list && echo "---" && head -20 ./utils/pgindent/typedefs.list && echo "---" && grep -c "^" ./utils/pgindent/typedefs.listRepository: pgEdge/spock
Length of output: 288
🏁 Script executed:
# Check if there are any comments in run-pgindent.sh about known issues or PG version specifics
cat ./utils/pgindent/run-pgindent.sh | grep -i "version\|todo\|fixme\|note"Repository: pgEdge/spock
Length of output: 38
🏁 Script executed:
# Double-check by searching for RepOriginId in the typedefs.list one more time to be absolutely certain
grep "RepOriginId" ./utils/pgindent/typedefs.listRepository: pgEdge/spock
Length of output: 38
RepOriginId missing from pgindent typedefs list
The pointer spacing RepOriginId * local_origin indicates that RepOriginId is not recognized as a typedef by pgindent. This type is a PostgreSQL core type from replication/origin.h and should appear in the combined typedefs list used by pgindent.
Update utils/pgindent/run-pgindent.sh to ensure RepOriginId is included when fetching core typedefs from the PostgreSQL buildfarm, or explicitly add it to the typedefs extraction if it's not being pulled automatically.
🤖 Prompt for AI Agents
In `@src/spock_conflict.c` at line 182, The pgindent run is treating "RepOriginId"
as a non-typedef (producing pointer spacing "RepOriginId * local_origin"); add
RepOriginId to the typedefs list used by pgindent so it is recognized as a core
typedef. Modify utils/pgindent/run-pgindent.sh to include RepOriginId when
fetching/combining core typedefs from the buildfarm (or add it explicitly to the
extraction list), ensuring the combined typedefs file used by pgindent contains
"RepOriginId" so functions like the one declaring RepOriginId * local_origin are
formatted correctly.
rasifr
force-pushed
the
task/code-indent
branch
from
b7100ed to
b5170cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spock_group.c (1)
323-328:⚠️ Potential issue | 🟡 Minor
Assert(sap != NULL)at line 328 is checked aftersapis already dereferenced on lines 326–327.The null-pointer assertion is placed after the
sap->key.*dereferences, making it ineffective. Move it before the first dereference.Proposed fix
SpockGroupEntry *entry; bool found; + Assert(sap != NULL); Assert(OidIsValid(sap->key.dbid) && OidIsValid(sap->key.node_id) && OidIsValid(sap->key.remote_node_id)); - Assert(sap != NULL);
🤖 Fix all issues with AI agents
In `@src/compat/18/spock_compat.c`:
- Around line 132-136: Remove the stray leading space before the block comment
opening so it starts with "/*" instead of " /*"; locate the comment that
precedes the check_simple_rowfilter_expr_walker description (the block
mentioning "Check if the row filter expression is a "simple expression"." and
referencing check_simple_rowfilter_expr_walker) and normalize it to match other
top-level block comments in the file.
In `@utils/pgindent/README.md`:
- Around line 3-6: The README claims run-pgindent.sh excludes spockctrl but the
script's find invocation (find ../../ -type f \( -name "*.c" -o -name "*.h" \))
has no exclusion; either update run-pgindent.sh to add an exclusion (e.g., add
-not -path '*/spockctrl/*' to the find command) so spockctrl is actually
skipped, or remove/update the "excluding spockctrl" sentence in
utils/pgindent/README.md to reflect current behavior; locate the find usage in
run-pgindent.sh and the corresponding README line to apply the change.
In `@utils/pgindent/run-pgindent.sh`:
- Around line 9-11: The three grep pipelines (the lines calling grep -nri
"typedef struct" ..., grep -nri "typedef enum" ..., and grep -nri "typedef
union" ...) can return exit code 1 when there are no matches which, under set
-euo pipefail, will abort the script; wrap each pipeline so a zero-match doesn't
fail the script—for example, append "|| true" to the end of each pipeline or
wrap the entire pipeline in "( ... ) || true" so the pipeline exit status is
ignored and typedefs.list is unchanged when there are no matches.
🧹 Nitpick comments (2)
src/spock_autoddl.c (1)
226-232: Blank line betweenifcondition and its body is confusing.After
if (targetrel == NULL), the blank line at 227 followed by a multi-line comment beforereturnmakes it easy to misread the control flow. Consider wrapping in braces for clarity, which would also be more robust against future edits.Suggested improvement
- if (targetrel == NULL) - - /* - * If relation doesn't exist - quietly exit. It is assumed that - * the core already produced an INFO message. - */ - return; + if (targetrel == NULL) + { + /* + * If relation doesn't exist - quietly exit. It is assumed that + * the core already produced an INFO message. + */ + return; + }src/spock_group.c (1)
422-429: Same readability concern: bracelesselsewith comment block before its statement.Similar to the pattern in
spock_autoddl.c, thiselseblock has a blank line + multi-line comment before the actualelogstatement, making control flow hard to follow. Consider adding braces.Suggested improvement
else - - /* - * Should never happen. In production just send the worker into - * exception behaviour without crash. - */ - elog(ERROR, "apply worker has not been fully initialised yet"); + { + /* + * Should never happen. In production just send the worker into + * exception behaviour without crash. + */ + elog(ERROR, "apply worker has not been fully initialised yet"); + }
- Run pgindent to standardize code formatting
rasifr
force-pushed
the
task/code-indent
branch
from
b5170cb to
3034e70
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
utils/pgindent/README.md (3)
3-6: Clarify the working directory for running the script.The instructions don't specify where to run
./run-pgindent.sh. Users may not realize they should execute it from theutils/pgindent/directory.📝 Suggested clarification
-1. Run `./run-pgindent.sh` to format all code. This script will: +1. From the `utils/pgindent/` directory, run `./run-pgindent.sh` to format all code. This script will:
8-13: Explicitly state that PostgreSQL source code is required.The prerequisites mention navigating to
src/tools/pg_bsd_indentand accessingsrc/tools/pgindent/pgindentfrom PostgreSQL source, but don't explicitly state that users need to have the PostgreSQL source code cloned first.📝 Suggested addition
2. Prerequisites: + - Clone PostgreSQL source code (if not already available) - Install pg_bsd_indent: navigate to src/tools/pg_bsd_indent in your postgres source code and run:
15-15: Improve wording for clarity.The phrase "struct set of the core and the Spock" is awkward:
- The file tracks typedefs, not structs directly
- "the Spock" should be "Spock" (proper noun)
- "the core" should be "PostgreSQL core" for clarity
📝 Suggested improvement
-3. File typedefs.list stays in repository to log changes in the struct set of the core and the Spock. +3. File typedefs.list stays in repository to log changes in the typedef set of PostgreSQL core and Spock.src/spock_apply_heap.c (1)
1204-1210: Inconsistent closing-paren alignment onspock_report_conflictcall.The closing
);on line 1210 is indented at\t\t\t\tlevel, which doesn't align with either the function name or the argument list. Compare with the otherspock_report_conflictcall at lines 1238–1242 where the closing paren is on the same line as the last argument. This may be a pgindent artifact, but it's worth checking if the indent tool produces this output or if it's a manual edit.Suggested fix — move closing paren to the last argument line
spock_report_conflict(SPOCK_CT_DELETE_LATE, rel, TTS_TUP(localslot), oldtup, NULL, /* remotetuple */ local_tuple, SpockResolution_Skip, xmin, local_origin_found, local_origin, - local_ts, edata->targetRel->idxoid - ); + local_ts, edata->targetRel->idxoid);src/spock_autoddl.c (1)
226-232: Blank line betweenifcondition and its body reduces readability.The blank line on line 227 between
if (targetrel == NULL)and the comment/returnstatement makes it easy to miss that thereturnis the body of theif. Consider wrapping in braces for clarity, or at minimum removing the blank line.Suggested fix — add braces for clarity
if (targetrel == NULL) - + { /* * If relation doesn't exist - quietly exit. It is assumed that * the core already produced an INFO message. */ return; + }
2 participants
No description provided.