Fix PostgreSQL 17 Building Failure by Hiding query->mergeUseOuterJoin by Vonng · Pull Request #74 · documentdb/documentdb · GitHub
Skip to content

Fix PostgreSQL 17 Building Failure by Hiding query->mergeUseOuterJoin - #74

Closed
Vonng wants to merge 1 commit into
documentdb:mainfrom
Vonng:main
Closed

Vonng wants to merge 1 commit into
documentdb:mainfrom
Vonng:main

Conversation

@Vonng

@Vonng Vonng commented Feb 21, 2025 •

Copy link
Copy Markdown

Related Issue: #73

query->mergeUseOuterJoin is removed in postgres 17

The following marco fix the build issue against PostgreSQL 17

	#if PG_VERSION_NUM < 170000
	query->mergeUseOuterJoin = true;
	#endif

You may have to replace it with the PG 17 new fields in the future.

	int  mergeTargetRelation pg_node_attr(query_jumble_ignore);

	/* join condition between source and target for MERGE */
	Node *mergeJoinCondition;

But for now, at least it can pass the building process for PG 17.

- query->mergeUseOuterJoin is removed in postgres 17
@Vonng
Vonng requested a review from a team as a code owner February 21, 2025 03:18
@safern

safern commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

@safern

safern commented Feb 26, 2025

Copy link
Copy Markdown
Contributor

@safern safern closed this Feb 26, 2025
guanzhousongmicrosoft added a commit that referenced this pull request Aug 22, 2026
…lease

Squashes the OSS-only fixes prepared for v0.116-0 on top of the internal
cherry-picks (documentdb-local, packaging and gateway) already synced from
the internal repo.

* fix(documentdb-local): interlock the data directory across containers.
  PostgreSQL decides whether postmaster.pid is stale from kill(pid, 0) and
  the SysV shared-memory segment. Both are PID-namespace-scoped, so inside
  containers both are wrong in both directions: a recycled PID makes a dead
  postmaster look alive (#43), and a live postmaster in another container
  looks dead (#62). Ownership is no longer decided from a PID. It is taken
  with an flock(2) claim on the data directory inode, which crosses
  namespaces and which the kernel releases when the holder dies, so a
  leftover postmaster.pid is residue by construction. A second container
  started on a volume already in use now refuses to start and names the
  conflict, instead of deleting the live lock file and taking both
  databases down while both containers still report Running=true.
  Fixes #43. Fixes #62.

* fix(emulator): exit on a bare positional instead of spinning at 100% CPU.
  The argument loop's only fallback arm was `-*)`, so a token that does not
  start with `-` matched no arm at all. Nothing shifted, `$#` never
  decreased, and the loop never terminated: the container burned a full CPU
  core, emitted no diagnostic, never started, and still reported
  Running=true to Docker. A `*)` catch-all now reports the offending token
  on stderr and exits 1. Fixes #61.

* fix(ci): publish container signatures cosign v2 clients can verify.
  Pair --new-bundle-format=false with --use-signing-config=false so cosign
  v3 keeps publishing the legacy .sig tag, and assert that tag is present
  after signing so a regression fails the build rather than the consumer.

* fix(packaging): forward SOURCE_DATE_EPOCH into the build containers. The
  per-format workflows pin it to the commit timestamp, but only in the
  runner's environment; the extras are assembled on the host and do see it,
  while the extension and gateway packages are built inside Docker and the
  four package-producing `docker run` calls did not forward it. Unset in the
  container, deb_install_changelog falls back to `date -u` and stamps each
  cell's own HH:MM:SS into the shipped /usr/share/doc/<pkg>/changelog.gz.
  The gateway package is PG-agnostic, so the pg17 and pg18 cells emit the
  same filename and build_all_packages.yml requires those copies to be
  structurally identical, comparing every payload entry's size; gzip -9n of
  that changelog is 172-174 bytes depending only on the timestamp digits, so
  two cells agree only about half the time and the release bundle fails
  intermittently. Forwarded with the valueless `--env SOURCE_DATE_EPOCH`
  form, which passes the host value through when set and leaves it unset
  otherwise, so a local build behaves exactly as it does today and no
  illegal empty value is ever injected.

* docs(readme): persist data with a volume and document readiness in the
  quickstart. The quickstart's docker run mounted no volume, so data was
  silently lost on container removal, and readiness was never mentioned.
  Also corrects the credential and reset semantics. Fixes #39.

* Set the v0.116-0 release date and mark v0.115-0 as skipped. The v0.115-0
  release was skipped; its changes are included in v0.116-0.

Verified end to end against the 0.116.0 images on PG 15, 16, 17 and 18: a
bare positional exits 1 with a diagnostic at 0% CPU; a second container on a
live volume is refused while the first keeps serving with its postmaster.pid
untouched and 2000 documents intact; 10 of 10 unclean restart cycles boot,
clear the residue and preserve the data. Entrypoint unit suite: 134 tests
pass. cosign v2.4.1 verifies the published manifests. Driving
deb_install_changelog as two matrix cells one second apart: 6 of 12 pairs
disagreed in size before the pin, 0 of 12 after.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Guanzhou Song <guanzhousong@microsoft.com>

---

Add RHEL prerequisite validation and extraction scripts for DocumentDB packaging

- Introduced `extract-el-prereqs.sh` to extract and validate RHEL prerequisite commands from README.md.
- Added `test-documented-prereqs.sh` to verify that documented prerequisites are sufficient for RPM installation on stock EL hosts.
- Updated workflows to utilize the new scripts for generating and checking prerequisites, ensuring consistency and preventing drift.

---

Fix review findings in the EL prerequisite tooling (GitHub issue #75)

Review findings folded in rather than left as a follow-up:

User-facing correctness:
- Document the real-RHEL path. `dnf config-manager --set-enabled crb` fails on
  subscribed RHEL, which has no `crb` repo id; the block now also carries the
  `subscription-manager repos --enable codeready-builder-...` form. Comment
  lines are inert when the block is executed, so it stays runnable verbatim.

Stop a job summary from gating the release:
- build_all_packages.yml's summary step is presentation-only and is the LAST
  step of `aggregate`, a `needs:` of create-release, running after the bundle
  is built and uploaded. It now warns and omits instead of exit 1.
  documentdb_release.yml still hard-fails, so the release path stays gated.

Make the suite stop lying about why something failed:
- A failing prerequisite command cascaded into a depsolve error, which the
  classifier refuses to retry and reported as "the docs are insufficient".
  The runner now fails fast and reports the prerequisite as the cause.
- The negative control matched libqhull_r BEFORE checking for a depsolve
  signature, so a download-phase failure whose transaction table merely lists
  libqhull_r was certified as "issue #75 reproduces". Reordered.

Guards that did not guard:
- One tripwire per documented command, not two. A deletion was invisible:
  --check-copies stays green (other copies remain a superset) and the install
  test does not backstop `module disable postgresql`.
- --check-copies scoped to %description. It grepped the whole spec, so moving
  the guidance to RPM comments (never shipped) still reported OK.
- Assert exactly one BEGIN/END marker pair; a stray marker silently
  concatenated unrelated README lines into the executed command list.
- Fail closed when the EL/arch rewrite does not match, instead of emitting an
  internally contradictory block.
- Normalise EL major and arch when comparing copies, so an arch-correct
  %description is not reported as drift.

Portability and hygiene:
- Anchor the PREREQ_EXIT/INSTALL_EXIT greps at column 0: `set -x` echoes the
  trap definition, whose text contains the marker, so every run reported a
  prerequisite failure that never happened.
- Use `sed -E`; BSD sed treats `\+` as a literal plus and skipped the rewrite.
- Absolutise the RPM path before docker sees it (a relative --packages-dir was
  read as a named volume).
- Exclude debuginfo/debugsource siblings from the RPM glob.
- Scope the drift gate to one matrix cell and note that this workflow's push
  trigger path-ignores '**/*.md'.

Verified on stock images against the real spec Requires set: EL9/x86_64 PG17
and PG18, EL9/aarch64 PG18, EL8/x86_64 PG17. Each also reproduces the #75
failure when CRB/powertools is removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LRK9WmRwAerTi9Am5ogbGH

---

Create documentdb_extended_rum on the packaged install path

documentdb-tune writes documentdb.alternate_index_handler_name='extended_rum'
whenever the extended-RUM control file is present on the host, but the access
method that GUC names only exists in a database where
documentdb_extended_rum has been created. That is per-database catalog state
no config file can supply, and CREATE EXTENSION documentdb CASCADE does not
pull it in. The result was a tuned cluster where every createIndexes failed
with "Index access method extended_rum is not available".

The control file ships inside postgresql-N-documentdb itself, so the probe is
always true and the GUC is always written. The container and the
documentdb-setup wizard were unaffected because both already create the
extension; only the documented manual path broke.

Adding documentdb_extended_rum to documentdb.control's requires is not a
viable fix: the extension errors out of _PG_init unless it is already in
shared_preload_libraries, which CASCADE cannot arrange.

- Single-source the extension-creation guidance in documentdb-tools-lib.sh so
  the tools cannot drift on which extensions a tuned cluster needs.
- documentdb-tune now prints the exact CREATE EXTENSION command, including
  documentdb_extended_rum when it wrote the extended-RUM GUCs.
- documentdb-createcluster --start creates the extensions, so the documented
  one-step path yields a cluster that can create indexes.
- documentdb-gateway-admin --check and documentdb-register-gateway no longer
  tell operators to run only CREATE EXTENSION documentdb CASCADE, which is
  what produced the broken state.
- Correct Workflows A and B in packaging-design.md and packaging/README.md.
- Add package-test coverage: assert index creation works end to end, and
  assert documentdb-tune's output names documentdb_extended_rum. The existing
  suite only exercised the wizard path, which is why this was not caught.

Fixes #74

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 729aef82-6b91-41ae-b513-542103e879d3
Signed-off-by: Guanzhou Song <guanzhousong@microsoft.com>

---

Keep psql stderr out of the index-probe assertion

verify_index_creation_works captured psql output with 2>&1, so the server's
"NOTICE:  creating collection" was spliced into the value compared against
'ok'. The index was created successfully; only the assertion failed.

Silence the notice with client_min_messages, and redirect stderr to a temp
file instead of merging it, so the captured value is the ok flag alone while
failures still report the backend error.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 729aef82-6b91-41ae-b513-542103e879d3
Signed-off-by: Guanzhou Song <guanzhousong@microsoft.com>

---

Resolve the extension hint against the cluster the tool is talking about

The extension-creation guidance added earlier in this change was correct only
on a single-cluster host with a live, restarted server. Three gaps:

1. The printed psql command carried no -h/-p/--cluster, so on a host with
   more than one cluster -- or the stand-alone documentdb-local instance on
   port 9700+major -- it silently created the extensions somewhere else and
   reported success. documentdb_create_extension_command now takes an
   optional conn_spec and every call site passes the coordinates it already
   held. documentdb-tune additionally reads the data directory's owner
   instead of assuming "postgres", which fails peer auth on those instances.

2. The handler probe read documentdb.alternate_index_handler_name and
   treated an empty result as "no extra extension needed". current_setting()
   returns NULL for an unknown GUC, and pg_documentdb is not preloaded until
   PostgreSQL restarts -- exactly the window documentdb-register-gateway
   warns in -- so the advice reverted to the single-statement recipe when it
   mattered most. The GUC's own default is "" too, so the two states had to
   be told apart rather than merged: the read now returns 2 for an absent
   GUC and falls back to the same on-disk control-file probe documentdb-tune
   used to decide to pin the handler.

3. documentdb-gateway-admin's new access-method probe used
   `2>/dev/null || true`, so a permission error or dropped connection printed
   a confident "Index creation will fail" about a healthy database. It now
   captures stderr separately and dies on a probe failure, matching the
   pattern already used in cmd_reset_password.

Also: documentdb-tune printed its guidance before the restart instruction on
the "config is already up to date" path, and under --dry-run, and claimed the
extensions belong in "each database" when the managed block pins
cron.database_name='postgres' and documentdb requires pg_cron -- so only the
postgres database can work. The restart guidance is extracted so every path
prints it first, and a --no-next-steps flag lets documentdb-createcluster
suppress tune's copy: with --start it has already created the extensions, and
without --start tune's copy landed above "Start with: pg_ctlcluster ...",
telling the operator to run CREATE EXTENSION against a stopped cluster.

documentdb-createcluster keeps failing open when the share directory cannot
be read, now with the reasoning recorded: documentdb-tune's resolve_pg_sharedir
swallows the identical failure and therefore does not pin the handler either,
so skipping the extension stays consistent with the config that was written.
It gains a psql preflight and prints the manual command when --start's
extension creation fails, where the cluster is already created and running and
the recovery hint on the no-start path is unreachable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wf1jNJ5hYsw2KV4NQeqjAQ

---

Fix the package banners that still print the single-statement recipe

The gateway package banners were missed entirely by the extended-RUM pass
above. Both the DEB postinst and the RPM %post scriptlet still printed

    sudo -u postgres psql -c 'CREATE EXTENSION documentdb CASCADE;'

inside their Workflow B recipe -- the highest-traffic copy in the repo, and
the one an operator sees on `apt install documentdb-gateway`. Every step of
that chain succeeds; the failure surfaces much later on the first
createIndexes, with nothing in the install output hinting a second statement
was needed. That also falsified the "single-sourced so those hints cannot
drift" claim in documentdb-tools-lib.sh: the drift was present at merge.

These two copies cannot source documentdb-tools-lib.sh -- it ships in
documentdb-postgresql-tools, which need not be installed when the scriptlet
runs -- so they stay static duplicates. verify_install_banner_extension_hint
in both packaged-install E2E suites now asserts them against the INSTALLED
scriptlet (/var/lib/dpkg/info/*.postinst, rpm -q --scripts), which checks the
shipped artifact rather than the source tree, and rejects a partial revert to
the single-statement form.

Coverage for the behaviour this PR advertises:

- documentdb-createcluster --start had never been executed anywhere. The only
  invocation in the repo is e2e-packaging-smoke-test.sh, without --start, in
  the non-Debian branch that exits early with its status discarded -- and
  packaging-design.md deleted the manual CREATE EXTENSION fallback in the same
  PR, so a broken --start would have left the documented path with no escape
  hatch. verify_createcluster_start_creates_extensions builds a throwaway
  cluster, asserts both extensions exist in THAT cluster, asserts an index
  actually builds, and checks the no-start banner is runnable top to bottom
  (start command first, recipe printed exactly once, naming --cluster N/name).

- The index probe selected only `ok`. ok=false is reported in-band -- psql
  exits 0 with empty stderr and the reason lives in retval -- so the single
  regression that test exists to catch reported as "got: 'f'", naming neither
  the access method nor the missing extension. It now selects retval too.

packaging/README.md framed `documentdb-createcluster 18 main --start` as a
one-step form of Workflow A, but postgresql-18 auto-creates cluster main on
install, so pg_createcluster aborts with "cluster 18/main already exists".
Restored the "starting from scratch" qualifier that packaging-design.md kept.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wf1jNJ5hYsw2KV4NQeqjAQ

---

Fix ordering, coordinates, and preflight gaps found reviewing the fixes

Five defects from a cold review of the two preceding sections, all in the same
class the original PR was fixing -- guidance that cannot be followed as
printed:

- documentdb-register-gateway printed the extended_rum recipe followed only by
  a reload. That warning fires in the post-tune/pre-restart window (the window
  the new on-disk fallback exists to serve), where pg_documentdb_extended_rum
  is not yet in shared_preload_libraries, so the statement errors out of
  _PG_init -- and a reload never applies shared_preload_libraries. It now
  states the restart precondition before the recipe, which is a no-op for an
  operator who already restarted.

- --no-next-steps suppressed only the CREATE EXTENSION hint, not
  print_restart_instruction, so documentdb-createcluster without --start
  printed "Restart the cluster to apply: systemctl restart postgresql@18-foo"
  and then "Start with: pg_ctlcluster 18 foo start" -- two different commands
  for a cluster that has never been started. The flag now means the wrapper
  owns all post-apply guidance, restart line included.

- The gateway banners were the one recipe in this change that gained no
  cluster coordinates. Every other line is parameterised on <N>, but a bare
  psql goes to whatever owns the default socket, so on a host where a
  different major is the default the extensions land there and the
  documentdb-register-gateway line below reports them missing. The DEB banner
  now passes --cluster <N>/main; the RPM one keeps a bare psql (--cluster is
  Debian pg_wrapper only) with the multi-major caveat on a separate advisory
  line -- not an inline comment, since a '#' inside a backslash-continued
  command comments out the continuation and truncates the recipe when pasted.
  verify_install_banner_extension_hint now asserts the coordinates too; it
  would not have caught this.

- documentdb-createcluster's psql preflight was unconditional and ran before
  argument parsing, so the no-start path -- which never invokes psql -- began
  dying on hosts without a client package. Moved after parsing and gated on
  --start.

- The new --start probe merged stderr into the captured row and then
  prefix-matched "t|", so any WARNING ahead of the row would fail the test on
  a healthy cluster. Now uses the same separate-stderr-file handling as the
  sibling verify_index_creation_works, which documents that exact reason.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wf1jNJ5hYsw2KV4NQeqjAQ

---

Compact the comments added by the extended-RUM fixes

Comment-only pass over the three preceding sections: same facts, roughly a
third fewer lines. The reasoning that is not recoverable from the code is
kept -- why the fail-open probe is deliberate, why the absent-vs-empty GUC
split exists, why the banners are static duplicates, why a '#' cannot go
inside the RPM recipe -- while the restatements of what the code plainly does
are gone.

No functional change: comment lines and blanks stripped, every touched file
hashed identically before and after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wf1jNJ5hYsw2KV4NQeqjAQ

---

Quiet psql in the createcluster probe so the result row stands alone

Found by running the packaged-install E2E: the new --start probe captured
"SET\nSET\nt|BSONHEX..." and failed its `t|` prefix match. Without -q psql
prints a command tag for each SET ahead of the result row. run_psql already
uses -Atq for exactly this reason; the new probe was the only psql call in the
suite that omitted it.

The code under test was correct -- the same run reported numIndexesBefore=1,
numIndexesAfter=2, ok=1, so documentdb-createcluster --start had created both
extensions and the index built.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wf1jNJ5hYsw2KV4NQeqjAQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants