MisterRaindrop commented on PR #1842: URL: https://github.com/apache/cloudberry/pull/1842#issuecomment-5278710109
@tuhaihe as you asked, the extension work now lives in this PR instead of a separate one, so the review context stays in one place. @andr-sokolov @yjhjstz please note what changed below before looking at the diff again. **How it was moved, and why this way.** The branch was *not* force-pushed. It now has, in order: 1. the original 26 kernel commits, untouched -- so your 64 inline comments stay anchored to the code they were written about; 2. `Revert the kernel-side lake-table DDL scaffolding` -- one commit that undoes all of them. The tree after it is identical to the merge base, verified by comparing tree object ids rather than by reading a diff; 3. the two extension commits, moved over from #1881. The resulting diff is identical to what #1881 showed -- I compared the two patches line by line; the only differences are blob hashes and one hunk offset, because the two branches had different merge bases. Rewriting the branch would have been shorter, but it would have orphaned the review history, and it would have made going back expensive. This way the kernel scaffolding is one `git revert` away if the extension approach turns out to be wrong. The pre-revert tip is also kept as `archive/iceberg-ddl-kernel-pr1842` on my fork. **Please re-review from scratch.** The diff is now 100% different from what was reviewed and approved earlier in this thread, so the earlier approval does not apply to what is here now. I would rather say that explicitly than have it merged on the strength of it. **One thing that will look out of place:** the second commit changes two lines in `contrib/interconnect/sql/interconnect.sql`. That test restarts the cluster with `gpstop -raiq`; an immediate shutdown skips the shutdown checkpoint, so the next startup runs crash recovery, and while the postmaster is in `PM_RECOVERY` it rejects the connection `gpstart` makes to read the segment configuration. `gpstop -r` then exits CRITICAL, psql gives up at the `\c` that follows, the whole file is skipped, and the cluster is left with only the coordinator running -- which is why `pg_trgm` and `indexscan` failed after it in the same job. A fast shutdown checkpoints on the way out, so that state is never entered. Measured with recovery deliberately slowed: `-raiq` failed 2/2, `-rafq` passed 3/3. Fast is what the rest of the tree already uses; this file was the only `-raiq`. Happy to split it into its own PR if you would rather keep this one to the extension. The original description is kept at the bottom of the PR body, struck through. #1881 is superseded by this PR. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
