Make installcheck step 1 #293
Open
+292
−137
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danolivo
force-pushed
the
make-installcheck-step-1
branch
3 times, most recently
from
57d932c to
a8e448b
Compare
Allow PostgreSQL's configure script to auto-detect llvm-config instead of hardcoding the path to /usr/bin/llvm-config-64. This improves portability across different systems and LLVM installations, as the hardcoded path may not exist on all distributions. The --with-llvm flag is sufficient for configure to find the appropriate llvm-config binary automatically. Based on commit e41e201 from dockerfile-stable-llvm-fix branch.
Replace the anti-pattern of setting environment variables via .bashrc with proper Docker ENV directives. This ensures: - Environment variables are available in all RUN commands - No need to source .bashrc in build steps - More reliable and Docker-native approach - Cleaner and more maintainable Dockerfile Variables moved to ENV: - PATH: Includes PostgreSQL bin directory - LD_LIBRARY_PATH: Includes PostgreSQL lib directory - PG_CONFIG: Points to pg_config binary This simplifies the Spock build step significantly.
Reformat the configure command from a single unreadable line with eval and quoted options to a clean multi-line format. Changes: - Remove unnecessary eval (potential security concern) - Remove shell variable with quoted options pattern - Use direct ./configure with multi-line arguments - Each option on its own line for easy review - Improved maintainability for future option changes This makes it much easier to: - Review which features are enabled - Add or remove configure options - Understand the build configuration at a glance
Merge multiple RUN commands into single layers to reduce the number of layers in the final Docker image. This optimization: - Reduces image size by combining related operations - Improves build efficiency - Better utilizes Docker layer caching - Follows Dockerfile best practices Changes: - PostgreSQL clone + chmod: 2 RUN → 1 RUN - pgedge setup: 3 RUN → 1 RUN - PostgreSQL compile: 2 RUN → 1 RUN - Spock compile: 3 RUN → 1 RUN - Added descriptive comments for each build stage - Removed duplicate WORKDIR directive Total reduction: ~6 fewer layers
Introduce a build argument to control the number of parallel make jobs
instead of using hardcoded values that differed between PostgreSQL
and Spock builds.
Changes:
- Add ARG MAKE_JOBS=4 (default value)
- Replace hardcoded -j4 (PostgreSQL) with -j${MAKE_JOBS}
- Replace hardcoded -j16 (Spock) with -j${MAKE_JOBS}
- Ensures consistent parallelism across all builds
Benefits:
- Can override at build time: --build-arg MAKE_JOBS=16
- Consistent behavior between PostgreSQL and Spock
- Easy to tune for different build environments
- Default of 4 is conservative and works on most systems
The previous inconsistency (j4 vs j16) is now eliminated.
Replace the two-step pattern of COPY + RUN chmod with the modern COPY --chmod directive available in BuildKit. Changes: - COPY --chmod=755 instead of COPY + RUN sudo chmod +x - Eliminates one RUN layer - Removes unnecessary sudo usage Benefits: - One fewer layer in the final image - Cleaner and more concise Dockerfile - Uses modern Docker/BuildKit features - Atomic operation (no window where files lack execute permission) This is the recommended approach in modern Dockerfiles.
Major improvements to the base image to fix several critical issues: 1. **Inline package list** - Removed dependency on lib-list.txt file - Eliminates need for build context with external files - Base image can now be built independently - All dependencies explicitly listed in Dockerfile 2. **Fix SSH key location** - SSH keys now created for pgedge user - Previously created in /root/.ssh (broken!) - Now correctly created in /home/pgedge/.ssh - Proper permissions (700 for .ssh, 600 for files) - Actually usable for SSH-based testing 3. **Set WORKDIR** - Added WORKDIR /home/pgedge - Child images no longer need to set it immediately - More sensible default than / 4. **Remove unused directory creation** - Removed mkdir /home/pgedge/spock - Child images create this when COPYing anyway - Eliminates wasted layer 5. **Better documentation** - Enhanced header comments - Inline comments for each section - Additional LABEL for description - Clarifies image purpose and contents 6. **Cleanup dnf cache** - Added dnf clean all - Reduces final image size This makes the base image self-contained and actually usable as a standalone build artifact.
Clarify and fix the user context throughout the build process to eliminate confusion and properly handle permissions. Changes: 1. **Explicit USER root at start** - Base image ends as USER pgedge - Step-1 needs root for installations - Now explicitly documented with comment 2. **Proper ownership after COPY** - Added chown after copying spock source - Ensures pgedge user can access files - Prevents permission issues during build 3. **Remove sudo, use su instead** - Changed: sudo -u pgedge → su - pgedge -c - More explicit about running as different user - Added chown before running install script 4. **Better comments on USER switching** - Documented why we switch to root - Documented why we switch back to pgedge - Makes build process clear Why this matters: - Eliminates confusion about current user context - Proper permissions throughout build - Better suited for CI/CD (GitHub Actions) - More maintainable and debuggable The image now has clear user context at each stage: - Start: root (for installations) - Runtime: pgedge (for testing)
Add spock.enable_quiet_mode GUC parameter to reduce message verbosity for cleaner output. When enabled, this parameter: 1. Downgrades DDL replication messages from INFO/WARNING to LOG level - "DDL statement replicated" (INFO → LOG) - "DDL statement replicated, but could be unsafe" (WARNING → LOG) - "This DDL statement will not be replicated" (WARNING → LOG) 2. Suppresses dependent object reporting in DROP CASCADE operations to reduce NOTICE message clutter The parameter defaults to false (disabled) for normal verbose output. Enable it by setting: spock.enable_quiet_mode = true This is useful for regression tests and production environments where less verbose output is desired. All diagnostic messages are preserved in the server log.
These two changes to the Spock initial script are intended to make ‘make installcheck’ clearer by avoiding false-positive errors. There is no evidence of the md5_agg_sfunc and md5_agg usage anywhere in the code - it seems it was an attempt at Spock objects naming. Also, add to the spock_gen_slot_name declaration the ‘PARALLEL SAFE’ clause just to keep regression tests quiet.
1. Bug. In the spock_repset.c module functions replication_set_add_table and replication_set_add_seq should check locking on the table before calling another lock-acquiring routine. It prevents unnecessary calls and reduces the lock level. 2. Improvement. In the spock_queue.c module function queue_message insert a tuple into the spock.queue table. The pattern of this table usage is quite trivial and doesn’t include any concurrent updates. So, we may release the table lock before commit without harm, and let ‘make installcheck’ be more conventional. author: Andrei Lepikhov co-author: Claude
PostgreSQL Core Patch Enhancement (pgXX-015-attoptions.diff): Added IsCatalogRelation() check in GetAttrDelta() function to prevent delta_apply attribute options from being applied to catalog relations. Rationale: - Catalog relations are never replicated via Logical Replication - No use case exists for delta_apply on system catalogs - Applying delta_apply to catalog tables would be meaningless - Early exit improves performance by avoiding unnecessary attribute scans Implementation: - Check added at the beginning of GetAttrDelta() - Returns NULL immediately if relation is a catalog - Includes clear comment explaining the rationale - No functional change for user tables Benefits: - Prevents accidental misconfiguration - Minor performance improvement for catalog operations - Clearer code intent through explicit check - Matches logical replication semantics Note: This patch is applied to PostgreSQL core during build to enable attribute-level options for delta_apply functionality.
In case of IF EXISTS / NOT EXISTS clause Postgres core utility call produces an INFO message and lets utility hooks do their job as usual. In the Spock case, we need to do the same and carefully process the clause so it doesn't produce an ERROR that breaks the convention.
danolivo
force-pushed
the
make-installcheck-step-1
branch
from
90c25cc to
f479e09
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series of commits makes 'make installcheck' visually clear.