From f99914d38f3d0dc77dceea4581a232a3dd0b9689 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 13 Jul 2026 09:08:45 -0400 Subject: [PATCH 1/8] ci: fork upstream auto-sync Keeps gburd/postgres rebased hourly on postgres/postgres master with only .github/ changes on top (sync-upstream automatic + manual). Drops the bespoke Windows dependency-builder workflow: Windows is already built and tested in CI by upstream's pg-ci.yml (Visual Studio + MinGW meson jobs), so a separate dependency prebuild that nothing consumed was redundant. --- .github/.gitignore | 18 + .github/QUICKSTART.md | 378 +++++++++++++++++++++ .github/README.md | 315 +++++++++++++++++ .github/docs/pristine-master-policy.md | 225 ++++++++++++ .github/docs/sync-setup.md | 326 ++++++++++++++++++ .github/workflows/sync-upstream-manual.yml | 249 ++++++++++++++ .github/workflows/sync-upstream.yml | 256 ++++++++++++++ 7 files changed, 1767 insertions(+) create mode 100644 .github/.gitignore create mode 100644 .github/QUICKSTART.md create mode 100644 .github/README.md create mode 100644 .github/docs/pristine-master-policy.md create mode 100644 .github/docs/sync-setup.md create mode 100644 .github/workflows/sync-upstream-manual.yml create mode 100644 .github/workflows/sync-upstream.yml diff --git a/.github/.gitignore b/.github/.gitignore new file mode 100644 index 0000000000000..a447f99442861 --- /dev/null +++ b/.github/.gitignore @@ -0,0 +1,18 @@ +# Node modules +scripts/ai-review/node_modules/ +# Note: package-lock.json should be committed for reproducible CI/CD builds + +# Logs +scripts/ai-review/cost-log-*.json +scripts/ai-review/*.log + +# OS files +.DS_Store +Thumbs.db + +# Editor files +*.swp +*.swo +*~ +.vscode/ +.idea/ diff --git a/.github/QUICKSTART.md b/.github/QUICKSTART.md new file mode 100644 index 0000000000000..d22c4d562ab7d --- /dev/null +++ b/.github/QUICKSTART.md @@ -0,0 +1,378 @@ +# Quick Start Guide - PostgreSQL Mirror CI/CD + +**Goal:** Get your PostgreSQL mirror CI/CD system running in 15 minutes. + +--- + +## āœ… What's Been Implemented + +- **Phase 1: Automated Upstream Sync** - Daily sync from postgres/postgres āœ… +- **Phase 2: AI-Powered Code Review** - Claude-based PR reviews āœ… +- **Phase 3: Windows Builds** - Planned for weeks 4-6 šŸ“‹ + +--- + +## šŸš€ Setup Instructions + +### Step 1: Configure GitHub Actions Permissions (2 minutes) + +1. Go to: **Settings ā†’ Actions ā†’ General** +2. Scroll to: **Workflow permissions** +3. Select: **"Read and write permissions"** +4. Check: **"Allow GitHub Actions to create and approve pull requests"** +5. Click: **Save** + +āœ… This enables workflows to push commits and create issues. + +--- + +### Step 2: Set Up Upstream Sync (3 minutes) + +**Test manual sync first:** + +```bash +# Via GitHub Web UI: +# 1. Go to: Actions tab +# 2. Click: "Sync from Upstream (Manual)" +# 3. Click: "Run workflow" +# 4. Watch it run (should take ~2 minutes) + +# OR via GitHub CLI: +gh workflow run sync-upstream-manual.yml +gh run watch +``` + +**Verify sync worked:** + +```bash +git fetch origin +git log origin/master --oneline -5 + +# Compare with upstream: +# https://github.com/postgres/postgres/commits/master +``` + +**Enable automatic sync:** + +- Automatic sync runs daily at 00:00 UTC +- Already configured, no action needed +- Check: Actions ā†’ "Sync from Upstream (Automatic)" + +āœ… Your master branch will now stay synced automatically. + +--- + +### Step 3: Set Up AI Code Review (10 minutes) + +**Choose Your Provider:** + +You can use either **Anthropic API** (simpler) or **AWS Bedrock** (if you have AWS infrastructure). + +#### Option A: Anthropic API (Recommended for getting started) + +**A. Get Claude API Key:** + +1. Go to: https://console.anthropic.com/ +2. Sign up or log in +3. Navigate to: API Keys +4. Create new key +5. Copy the key (starts with `sk-ant-...`) + +**B. Add API Key to GitHub:** + +1. Go to: **Settings ā†’ Secrets and variables ā†’ Actions** +2. Click: **New repository secret** +3. Name: `ANTHROPIC_API_KEY` +4. Value: Paste your API key +5. Click: **Add secret** + +**C. Ensure config uses Anthropic:** + +Check `.github/scripts/ai-review/config.json` has: +```json +{ + "provider": "anthropic", + ... +} +``` + +#### Option B: AWS Bedrock (If you have AWS) + +See detailed guide: [.github/docs/bedrock-setup.md](.github/docs/bedrock-setup.md) + +**Quick steps:** +1. Enable Claude 3.5 Sonnet in AWS Bedrock console +2. Create IAM user with `bedrock:InvokeModel` permission +3. Add three secrets to GitHub: + - `AWS_ACCESS_KEY_ID` + - `AWS_SECRET_ACCESS_KEY` + - `AWS_REGION` (e.g., `us-east-1`) +4. Update `.github/scripts/ai-review/config.json`: +```json +{ + "provider": "bedrock", + "bedrock_model_id": "us.anthropic.claude-3-5-sonnet-20241022-v2:0", + "bedrock_region": "us-east-1", + ... +} +``` + +**Note:** Both providers have identical pricing ($0.003/1K input, $0.015/1K output tokens). + +--- + +**C. Install Dependencies:** + +```bash +cd .github/scripts/ai-review +npm install + +# Should install: +# - @anthropic-ai/sdk (for Anthropic API) +# - @aws-sdk/client-bedrock-runtime (for AWS Bedrock) +# - @actions/github +# - @actions/core +# - parse-diff +# - minimatch +``` + +**D. Test AI Review:** + +```bash +# Option 1: Create a test PR +git checkout -b test/ai-review +echo "// Test change" >> src/backend/utils/adt/int.c +git add . +git commit -m "Test: AI review" +git push origin test/ai-review +# Create PR via GitHub UI + +# Option 2: Manual trigger on existing PR +gh workflow run ai-code-review.yml -f pr_number= +``` + +āœ… AI will review the PR and post comments + summary. + +--- + +## šŸŽÆ Verify Everything Works + +### Check Sync Status + +```bash +# Check latest sync run +gh run list --workflow=sync-upstream.yml --limit 1 + +# View details +gh run view $(gh run list --workflow=sync-upstream.yml --limit 1 --json databaseId -q '.[0].databaseId') +``` + +**Expected:** āœ… Green checkmark, "Already up to date" or "Successfully synced X commits" + +### Check AI Review Status + +```bash +# Check latest AI review run +gh run list --workflow=ai-code-review.yml --limit 1 + +# View details +gh run view $(gh run list --workflow=ai-code-review.yml --limit 1 --json databaseId -q '.[0].databaseId') +``` + +**Expected:** āœ… Green checkmark, comments posted on PR + +--- + +## šŸ“Š Monitor Costs + +### GitHub Actions Minutes + +```bash +# View usage (requires admin access) +gh api /repos/gburd/postgres/actions/cache/usage + +# Expected monthly usage: +# - Sync: ~150 minutes (FREE - within 2,000 min limit) +# - AI Review: ~200 minutes (FREE - within limit) +``` + +### Claude API Costs + +**View per-PR cost:** +- Check AI review summary comment on PR +- Format: `Cost: $X.XX | Model: claude-3-5-sonnet` + +**Expected costs:** +- Small PR: $0.50 - $1.00 +- Medium PR: $1.00 - $3.00 +- Large PR: $3.00 - $7.50 +- **Monthly (20 PRs):** $35-50 + +**Download detailed logs:** +```bash +gh run list --workflow=ai-code-review.yml --limit 5 +gh run download -n ai-review-cost-log- +``` + +--- + +## šŸ”§ Configuration + +### Adjust Sync Schedule + +Edit `.github/workflows/sync-upstream.yml`: + +```yaml +on: + schedule: + # Current: Daily at 00:00 UTC + - cron: '0 0 * * *' + + # Options: + # Every 6 hours: '0 */6 * * *' + # Twice daily: '0 0,12 * * *' + # Weekdays only: '0 0 * * 1-5' +``` + +### Adjust AI Review Costs + +Edit `.github/scripts/ai-review/config.json`: + +```json +{ + "cost_limits": { + "max_per_pr_dollars": 15.0, // ā† Lower this to save money + "max_per_month_dollars": 200.0, // ā† Hard monthly cap + "alert_threshold_dollars": 150.0 + }, + + "max_file_size_lines": 5000, // ā† Skip files larger than this + + "skip_paths": [ + "*.png", "*.svg", // Already skipped + "vendor/**/*", // ā† Add more patterns here + "generated/**/*" + ] +} +``` + +### Adjust AI Review Prompts + +**Make AI reviews stricter or more lenient:** + +Edit files in `.github/scripts/ai-review/prompts/`: +- `c-code.md` - PostgreSQL C code review +- `sql.md` - SQL and regression tests +- `documentation.md` - Documentation review +- `build-system.md` - Makefile/Meson review + +--- + +## šŸ› Troubleshooting + +### Sync Not Working + +**Problem:** Workflow fails with "Permission denied" + +**Fix:** +- Check: Settings ā†’ Actions ā†’ Workflow permissions +- Ensure: "Read and write permissions" is selected + +--- + +### AI Review Not Posting Comments + +**Problem:** Workflow runs but no comments appear + +**Check:** +1. Is PR a draft? (Draft PRs are skipped to save costs) +2. Are there reviewable files? (Check workflow logs) +3. Is API key valid? (Settings ā†’ Secrets ā†’ ANTHROPIC_API_KEY) + +**Fix:** +- Mark PR as "Ready for review" if draft +- Check workflow logs: Actions ā†’ Latest run ā†’ View logs +- Verify API key at https://console.anthropic.com/ + +--- + +### High AI Review Costs + +**Problem:** Costs higher than expected + +**Check:** +- Download cost logs: `gh run download ` +- Look for large files being reviewed +- Check number of PR updates (each triggers review) + +**Fix:** +1. Add large files to `skip_paths` in config.json +2. Lower `max_tokens_per_request` (shorter reviews) +3. Use draft PRs for work-in-progress +4. Batch PR updates to reduce review frequency + +--- + +## šŸ“š Full Documentation + +- **Overview:** [.github/README.md](.github/README.md) +- **Sync Guide:** [.github/docs/sync-setup.md](.github/docs/sync-setup.md) +- **AI Review Guide:** [.github/docs/ai-review-guide.md](.github/docs/ai-review-guide.md) +- **Windows Builds:** [.github/docs/windows-builds.md](.github/docs/windows-builds.md) (planned) +- **Implementation Status:** [.github/IMPLEMENTATION_STATUS.md](.github/IMPLEMENTATION_STATUS.md) + +--- + +## āœØ What's Next? + +### Immediate +- āœ… **Monitor first automatic sync** (tonight at 00:00 UTC) +- āœ… **Test AI review on real PR** +- āœ… **Tune prompts** based on feedback + +### This Week +- Shadow mode testing for AI reviews (Week 1) +- Gather developer feedback +- Adjust configuration + +### Weeks 2-3 +- Enable full AI review mode +- Monitor costs and quality +- Iterate on prompts + +### Weeks 4-6 +- **Phase 3:** Implement Windows dependency builds +- Research winpgbuild approach +- Create build workflows +- Test artifact publishing + +--- + +## šŸŽ‰ Success Criteria + +You'll know everything is working when: + +āœ… **Sync:** +- Master branch matches postgres/postgres +- Daily sync runs show green checkmarks +- No open issues with label `sync-failure` + +āœ… **AI Review:** +- PRs receive inline comments + summary +- Feedback is relevant and actionable +- Costs stay under $50/month +- Developers find reviews helpful + +āœ… **Overall:** +- Automation saves 8-16 hours/month +- Issues caught earlier in development +- No manual sync needed + +--- + +**Need Help?** +- Check documentation: `.github/README.md` +- Check workflow logs: Actions ā†’ Failed run ā†’ View logs +- Create issue with workflow URL and error messages + +**Ready to go!** šŸš€ diff --git a/.github/README.md b/.github/README.md new file mode 100644 index 0000000000000..bdfcfe74ac4a4 --- /dev/null +++ b/.github/README.md @@ -0,0 +1,315 @@ +# PostgreSQL Mirror CI/CD System + +This directory contains the CI/CD infrastructure for the PostgreSQL personal mirror repository. + +## System Overview + +``` +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ PostgreSQL Mirror CI/CD ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¼ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” + ā”‚ ā”‚ ā”‚ + [1] Sync [2] AI Review [3] Windows + Daily @ 00:00 On PR Events On Master Push + ā”‚ ā”‚ ā”‚ + ā–¼ ā–¼ ā–¼ + postgres/postgres Claude API Dependency Builds + ā”‚ ā”‚ ā”‚ + ā–¼ ā–¼ ā–¼ + github.com/gburd PR Comments Build Artifacts + /postgres/ + Labels (90-day retention) + master +``` + +## Components + +### 1. Automated Upstream Sync +**Status:** āœ“ Implemented +**Files:** `workflows/sync-upstream*.yml` + +Automatically syncs the `master` branch with upstream `postgres/postgres` daily. + +- **Frequency:** Daily at 00:00 UTC +- **Trigger:** Cron schedule + manual +- **Features:** + - Fast-forward merge (conflict-free) + - Automatic issue creation on conflicts + - Issue auto-closure on resolution +- **Cost:** Free (~150 min/month, well within free tier) + +**Documentation:** [docs/sync-setup.md](docs/sync-setup.md) + +### 2. AI-Powered Code Review +**Status:** āœ“ Implemented +**Files:** `workflows/ai-code-review.yml`, `scripts/ai-review/` + +Uses Claude API to provide PostgreSQL-aware code review on pull requests. + +- **Trigger:** PR opened/updated, ready for review +- **Features:** + - PostgreSQL-specific C code review + - SQL, documentation, build system review + - Inline comments on issues + - Automatic labeling (security, performance, etc.) + - Cost tracking and limits + - **Provider Options:** Anthropic API or AWS Bedrock +- **Cost:** $35-50/month (estimated) +- **Model:** Claude 3.5 Sonnet + +**Documentation:** [docs/ai-review-guide.md](docs/ai-review-guide.md) + +### 3. Windows Build Integration +**Status:** āœ… Implemented +**Files:** `workflows/windows-dependencies.yml`, `windows/`, `scripts/windows/` + +Builds PostgreSQL Windows dependencies for x64 Windows. + +- **Trigger:** Manual, manifest changes, weekly refresh +- **Features:** + - Core dependencies: OpenSSL, zlib, libxml2 + - Smart caching by version hash + - Dependency bundling + - Artifact publishing (90-day retention) + - PowerShell download helper + - **Cost optimization:** Skips builds for pristine commits (dev setup, .github/ only) +- **Cost:** ~$5-8/month (with caching and optimization) + +**Documentation:** [docs/windows-builds.md](docs/windows-builds.md) | [Usage](docs/windows-builds-usage.md) + +## Quick Start + +### Prerequisites + +1. **GitHub Actions enabled:** + - Settings ā†’ Actions ā†’ General ā†’ Allow all actions + +2. **Workflow permissions:** + - Settings ā†’ Actions ā†’ General ā†’ Workflow permissions + - Select: "Read and write permissions" + - Enable: "Allow GitHub Actions to create and approve pull requests" + +3. **Secrets configured:** + - **Option A - Anthropic API:** + - Settings ā†’ Secrets and variables ā†’ Actions + - Add: `ANTHROPIC_API_KEY` (get from https://console.anthropic.com/) + - **Option B - AWS Bedrock:** + - Add: `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, `AWS_REGION` + - See: [docs/bedrock-setup.md](docs/bedrock-setup.md) + +### Using the Sync System + +**Manual sync:** +```bash +# Via GitHub UI: +# Actions ā†’ "Sync from Upstream (Manual)" ā†’ Run workflow + +# Via GitHub CLI: +gh workflow run sync-upstream-manual.yml +``` + +**Check sync status:** +```bash +# Latest sync run +gh run list --workflow=sync-upstream.yml --limit 1 + +# View details +gh run view +``` + +### Using AI Code Review + +AI reviews run automatically on PRs. To test manually: + +```bash +# Via GitHub UI: +# Actions ā†’ "AI Code Review" ā†’ Run workflow ā†’ Enter PR number + +# Via GitHub CLI: +gh workflow run ai-code-review.yml -f pr_number=123 +``` + +**Reviewing AI feedback:** +1. AI posts inline comments on specific lines +2. AI posts summary comment with overview +3. AI adds labels (security-concern, needs-tests, etc.) +4. Review and address feedback like human reviewer comments + +### Cost Monitoring + +**View AI review costs:** +```bash +# Download cost logs +gh run download -n ai-review-cost-log- +``` + +**Expected monthly costs (with optimizations):** +- Sync: $0 (free tier) +- AI Review: $30-45 (only on PRs, skips drafts) +- Windows Builds: $5-8 (caching + pristine commit skipping) +- **Total: $35-53/month** + +**Cost optimizations:** +- Windows builds skip "dev setup" and .github/-only commits +- AI review only runs on non-draft PRs +- Aggressive caching reduces build times by 80-90% +- See [Cost Optimization Guide](docs/cost-optimization.md) for details + +## Workflow Files + +### Sync Workflows +- `workflows/sync-upstream.yml` - Automatic daily sync +- `workflows/sync-upstream-manual.yml` - Manual testing sync + +### AI Review Workflows +- `workflows/ai-code-review.yml` - Automatic PR review + +### Windows Build Workflows +- `workflows/windows-dependencies.yml` - Dependency builds (TBD) + +## Configuration Files + +### AI Review Configuration +- `scripts/ai-review/config.json` - Cost limits, file patterns, labels +- `scripts/ai-review/prompts/*.md` - Review prompts by file type +- `scripts/ai-review/package.json` - Node.js dependencies + +### Windows Build Configuration +- `windows/manifest.json` - Dependency versions (TBD) + +## Branch Strategy + +### Master Branch: Mirror Only +- **Purpose:** Pristine copy of `postgres/postgres` +- **Rule:** Never commit directly to master +- **Sync:** Automatic via GitHub Actions +- **Protection:** Consider branch protection rules + +### Feature Branches: Development +- **Pattern:** `feature/*`, `dev/*`, `experiment/*` +- **Workflow:** + ```bash + git checkout master + git pull origin master + git checkout -b feature/my-feature + # ... make changes ... + git push origin feature/my-feature + # Create PR: feature/my-feature ā†’ master + ``` + +### Special Branches +- `recovery/*` - Temporary branches for sync conflict resolution +- Development remotes: commitfest, heikki, orioledb, zheap + +## Integration with Cirrus CI + +GitHub Actions and Cirrus CI run independently: + +- **Cirrus CI:** Comprehensive testing (Linux, FreeBSD, macOS, Windows) +- **GitHub Actions:** Sync, AI review, Windows dependency builds +- **No conflicts:** Both can run on same commits + +## Troubleshooting + +### Sync Issues + +**Problem:** Sync workflow failing +**Check:** Actions ā†’ "Sync from Upstream (Automatic)" ā†’ Latest run +**Fix:** See [docs/sync-setup.md](docs/sync-setup.md#sync-failure-recovery) + +### AI Review Issues + +**Problem:** AI review not running +**Check:** Is PR a draft? Draft PRs are skipped +**Fix:** Mark PR as ready for review + +**Problem:** AI review too expensive +**Check:** Cost logs in workflow artifacts +**Fix:** Adjust limits in `scripts/ai-review/config.json` + +### Workflow Permission Issues + +**Problem:** "Resource not accessible by integration" +**Check:** Settings ā†’ Actions ā†’ General ā†’ Workflow permissions +**Fix:** Enable "Read and write permissions" + +## Security + +### Secrets Management +- `ANTHROPIC_API_KEY`: Claude API key (required for AI review) +- `GITHUB_TOKEN`: Auto-generated, scoped to repository +- Never commit secrets to repository +- Rotate API keys quarterly + +### Permissions +- Workflows use minimum necessary permissions +- `contents: read` for code access +- `pull-requests: write` for comments +- `issues: write` for sync failure issues + +### Audit Trail +- All workflow runs logged (90-day retention) +- Cost tracking for AI reviews +- GitHub Actions audit log available + +## Support and Documentation + +### Detailed Documentation +- [Sync Setup Guide](docs/sync-setup.md) - Upstream sync system +- [AI Review Guide](docs/ai-review-guide.md) - AI code review system +- [Windows Builds Guide](docs/windows-builds.md) - Windows dependencies +- [Cost Optimization Guide](docs/cost-optimization.md) - Reducing CI/CD costs +- [Pristine Master Policy](docs/pristine-master-policy.md) - Master branch management + +### Reporting Issues + +Issues with CI/CD system: +1. Check workflow logs: Actions ā†’ Failed run ā†’ View logs +2. Search existing issues: label:automation +3. Create issue with workflow run URL and error messages + +### Modifying Workflows + +**Disabling a workflow:** +```bash +# Via GitHub UI: +# Actions ā†’ Select workflow ā†’ "..." ā†’ Disable workflow + +# Via git: +git mv .github/workflows/workflow-name.yml .github/workflows/workflow-name.yml.disabled +git commit -m "Disable workflow" +``` + +**Testing workflow changes:** +1. Create feature branch +2. Modify workflow file +3. Use `workflow_dispatch` trigger to test +4. Verify in Actions tab +5. Merge to master when working + +## Cost Summary + +| Component | Monthly Cost | Usage | Notes | +|-----------|-------------|-------|-------| +| Sync | $0 | ~150 min | Free tier: 2,000 min | +| AI Review | $30-45 | Variable | Claude API usage-based | +| Windows Builds | $5-8 | ~2,500 min | With caching + optimization | +| **Total** | **$35-53** | | After cost optimizations | + +**Comparison:** CodeRabbit (turnkey solution) = $99-499/month + +**Cost savings:** ~40-47% reduction through optimizations (see [Cost Optimization Guide](docs/cost-optimization.md)) + +## References + +- PostgreSQL: https://github.com/postgres/postgres +- GitHub Actions: https://docs.github.com/en/actions +- Claude API: https://docs.anthropic.com/ +- Cirrus CI: https://cirrus-ci.org/ +- winpgbuild: https://github.com/dpage/winpgbuild + +--- + +**Last Updated:** 2026-03-10 +**Maintained by:** PostgreSQL Mirror Automation diff --git a/.github/docs/pristine-master-policy.md b/.github/docs/pristine-master-policy.md new file mode 100644 index 0000000000000..9c0479d32df6a --- /dev/null +++ b/.github/docs/pristine-master-policy.md @@ -0,0 +1,225 @@ +# Pristine Master Policy + +## Overview + +The `master` branch in this mirror repository follows a "mostly pristine" policy, meaning it should closely mirror the upstream `postgres/postgres` repository with only specific exceptions allowed. + +## Allowed Commits on Master + +Master is considered "pristine" and the sync workflow will successfully merge upstream changes if local commits fall into these categories: + +### 1. āœ… CI/CD Configuration (`.github/` directory only) + +Commits that only modify files within the `.github/` directory are allowed. + +**Examples:** +- Adding GitHub Actions workflows +- Updating AI review configuration +- Modifying sync schedules +- Adding documentation in `.github/docs/` + +**Rationale:** CI/CD configuration is repository-specific and doesn't affect the PostgreSQL codebase itself. + +### 2. āœ… Development Environment Setup (commits named "dev setup ...") + +Commits with messages starting with "dev setup" (case-insensitive) are allowed, even if they modify files outside `.github/`. + +**Examples:** +- `dev setup v19` +- `Dev Setup: Add debugging configuration` +- `DEV SETUP - IDE and tooling` + +**Typical files in dev setup commits:** +- `.clang-format`, `.clangd` - Code formatting and LSP config +- `.envrc` - Directory environment variables (direnv) +- `.gdbinit` - Debugger configuration +- `.idea/`, `.vscode/` - IDE settings +- `flake.nix`, `shell.nix` - Nix development environment +- `pg-aliases.sh` - Personal shell aliases +- Other personal development tools + +**Rationale:** Development environment configuration is personal and doesn't affect the code or CI/CD. It's frequently updated as developers refine their workflow. + +### 3. āŒ Code Changes (NOT allowed) + +Any commits that: +- Modify PostgreSQL source code (`src/`, `contrib/`, etc.) +- Modify tests outside `.github/` +- Modify build system outside `.github/` +- Are not `.github/`-only AND don't start with "dev setup" + +**These will cause sync failures** and require manual resolution. + +## Branch Strategy + +### Master Branch +- **Purpose:** Mirror of upstream `postgres/postgres` + local CI/CD + dev environment +- **Updates:** Automatic hourly sync from upstream +- **Direct commits:** Only `.github/` changes or "dev setup" commits +- **All other work:** Use feature branches + +### Feature Branches +- **Purpose:** All PostgreSQL development work +- **Pattern:** `feature/*`, `dev/*`, `experiment/*` +- **Workflow:** + ```bash + git checkout master + git pull origin master + git checkout -b feature/my-feature + # Make changes... + git push origin feature/my-feature + # Create PR: feature/my-feature ā†’ master + ``` + +## Sync Workflow Behavior + +### Scenario 1: No Local Commits +``` +Upstream: A---B---C +Master: A---B---C +``` +**Result:** āœ… Already up to date (no action needed) + +### Scenario 2: Only .github/ Commits +``` +Upstream: A---B---C---D +Master: A---B---C---X (X modifies .github/ only) +``` +**Result:** āœ… Merge commit created +``` +Master: A---B---C---X---M + \ / + D---/ +``` + +### Scenario 3: Only "dev setup" Commits +``` +Upstream: A---B---C---D +Master: A---B---C---Y (Y is "dev setup v19") +``` +**Result:** āœ… Merge commit created +``` +Master: A---B---C---Y---M + \ / + D---/ +``` + +### Scenario 4: Mix of Allowed Commits +``` +Upstream: A---B---C---D +Master: A---B---C---X---Y (X=.github/, Y=dev setup) +``` +**Result:** āœ… Merge commit created + +### Scenario 5: Code Changes (Violation) +``` +Upstream: A---B---C---D +Master: A---B---C---Z (Z modifies src/backend/) +``` +**Result:** āŒ Sync fails, issue created + +**Recovery:** +1. Create feature branch from Z +2. Reset master to match upstream +3. Rebase feature branch +4. Create PR + +## Updating Dev Setup + +When you update your development environment: + +```bash +# Make changes to .clangd, flake.nix, etc. +git add .clangd flake.nix .vscode/ + +# Important: Start message with "dev setup" +git commit -m "dev setup v20: Update clangd config and add new aliases" + +git push origin master +``` + +The sync workflow will recognize this as a dev setup commit and preserve it during merges. + +**Naming convention:** +- āœ… `dev setup v20` +- āœ… `Dev setup: Update IDE config` +- āœ… `DEV SETUP - Add debugging tools` +- āŒ `Update development environment` (doesn't start with "dev setup") +- āŒ `dev environment changes` (doesn't start with "dev setup") + +## Sync Failure Recovery + +If sync fails because of non-allowed commits: + +### Check What's Wrong +```bash +git fetch origin +git fetch upstream https://github.com/postgres/postgres.git master + +# See which commits are problematic +git log upstream/master..origin/master --oneline + +# See which files were changed +git diff --name-only upstream/master...origin/master +``` + +### Option 1: Make Commit Acceptable + +If the commit should have been a "dev setup" commit: + +```bash +# Amend the commit message +git commit --amend -m "dev setup v21: Previous changes" +git push origin master --force-with-lease +``` + +### Option 2: Move to Feature Branch + +If the commit contains code changes: + +```bash +# Create feature branch +git checkout -b feature/recovery origin/master + +# Reset master to upstream +git checkout master +git reset --hard upstream/master +git push origin master --force + +# Your changes are safe in feature/recovery +git checkout feature/recovery +# Create PR when ready +``` + +## FAQ + +**Q: Why allow dev setup commits on master?** +A: Development environment configuration is personal, frequently updated, and doesn't affect the codebase or CI/CD. It's more convenient to keep it on master than manage separate branches. + +**Q: What if I forget to name it "dev setup"?** +A: Sync will fail. You can amend the commit message (see recovery above) or move the commit to a feature branch. + +**Q: Can I have both .github/ and dev setup changes in one commit?** +A: Yes! The sync workflow allows commits that modify .github/, or are named "dev setup", or both. + +**Q: What if upstream modifies the same files as my dev setup commit?** +A: The sync will attempt to merge automatically. If there are conflicts, you'll need to resolve them manually (rare, since upstream shouldn't touch personal dev files). + +**Q: Can I reorder commits on master?** +A: It's not recommended due to complexity. The sync workflow handles commits in any order as long as they follow the policy. + +## Monitoring + +**Check sync status:** +- Actions ā†’ "Sync from Upstream (Automatic)" +- Look for green āœ… on recent runs + +**Check for policy violations:** +- Open issues with label `sync-failure` +- These indicate commits that violated the pristine master policy + +## Related Documentation + +- [Sync Setup Guide](sync-setup.md) - Detailed sync workflow documentation +- [QUICKSTART](../QUICKSTART.md) - Quick setup guide +- [README](../README.md) - System overview diff --git a/.github/docs/sync-setup.md b/.github/docs/sync-setup.md new file mode 100644 index 0000000000000..1e12aeea3c5fc --- /dev/null +++ b/.github/docs/sync-setup.md @@ -0,0 +1,326 @@ +# Automated Upstream Sync Documentation + +## Overview + +This repository maintains a mirror of the official PostgreSQL repository at `postgres/postgres`. The sync system automatically keeps the `master` branch synchronized with upstream changes. + +## System Components + +### 1. Automatic Daily Sync +**File:** `.github/workflows/sync-upstream.yml` + +- **Trigger:** Daily at 00:00 UTC (cron schedule) +- **Purpose:** Automatically sync master branch without manual intervention +- **Process:** + 1. Fetches latest commits from `postgres/postgres` + 2. Fast-forward merges to local master (conflict-free) + 3. Pushes to `origin/master` + 4. Creates GitHub issue if conflicts detected + 5. Closes existing sync-failure issues on success + +### 2. Manual Sync Workflow +**File:** `.github/workflows/sync-upstream-manual.yml` + +- **Trigger:** Manual via Actions tab ā†’ "Sync from Upstream (Manual)" ā†’ Run workflow +- **Purpose:** Testing and on-demand syncs +- **Options:** + - `force_push`: Use `--force-with-lease` when pushing (default: true) + +## Branch Strategy + +### Critical Rule: Master is Pristine + +- **master branch:** Mirror only - pristine copy of `postgres/postgres` +- **All development:** Feature branches (e.g., `feature/hot-updates`, `experiment/zheap`) +- **Never commit directly to master** - this will cause sync failures + +### Feature Branch Workflow + +```bash +# Start new feature from latest master +git checkout master +git pull origin master +git checkout -b feature/my-feature + +# Work on feature +git commit -m "Add feature" + +# Keep feature updated with upstream +git checkout master +git pull origin master +git checkout feature/my-feature +git rebase master + +# Push feature branch +git push origin feature/my-feature + +# Create PR: feature/my-feature ā†’ master +``` + +## Sync Failure Recovery + +### Diagnosis + +If sync fails, you'll receive a GitHub issue with label `sync-failure`. Check what commits are on master but not upstream: + +```bash +# Clone or update your local repository +git fetch origin +git fetch upstream https://github.com/postgres/postgres.git master + +# View conflicting commits +git log upstream/master..origin/master --oneline + +# See detailed changes +git diff upstream/master...origin/master +``` + +### Recovery Option 1: Preserve Commits (Recommended) + +If the commits on master should be kept: + +```bash +# Create backup branch from current master +git checkout origin/master +git checkout -b recovery/master-backup-$(date +%Y%m%d) +git push origin recovery/master-backup-$(date +%Y%m%d) + +# Reset master to upstream +git checkout master +git reset --hard upstream/master +git push origin master --force + +# Create feature branch from backup +git checkout -b feature/recovered-work recovery/master-backup-$(date +%Y%m%d) + +# Optional: rebase onto new master +git rebase master + +# Push feature branch +git push origin feature/recovered-work + +# Create PR: feature/recovered-work ā†’ master +``` + +### Recovery Option 2: Discard Commits + +If the commits on master were mistakes or already merged upstream: + +```bash +git checkout master +git reset --hard upstream/master +git push origin master --force +``` + +### Verification + +After recovery, verify sync status: + +```bash +# Check that master matches upstream +git log origin/master --oneline -10 +git log upstream/master --oneline -10 + +# These should be identical + +# Or run manual sync workflow +# GitHub ā†’ Actions ā†’ "Sync from Upstream (Manual)" ā†’ Run workflow +``` + +The automatic sync will resume on next scheduled run (00:00 UTC daily). + +## Monitoring + +### Success Indicators + +- āœ“ GitHub Actions badge shows passing +- āœ“ No open issues with label `sync-failure` +- āœ“ `master` branch commit history matches `postgres/postgres` + +### Check Sync Status + +**Via GitHub UI:** +1. Go to: Actions ā†’ "Sync from Upstream (Automatic)" +2. Check latest run status + +**Via Git:** +```bash +git fetch origin +git fetch upstream https://github.com/postgres/postgres.git master +git log origin/master..upstream/master --oneline + +# No output = fully synced +# Commits listed = behind upstream (sync pending or failed) +``` + +**Via API:** +```bash +# Check latest workflow run +gh run list --workflow=sync-upstream.yml --limit 1 + +# View run details +gh run view +``` + +### Sync Lag + +Expected lag: <1 hour from upstream commit to mirror + +- Upstream commits at 12:30 UTC ā†’ Synced at next daily run (00:00 UTC next day) = ~11.5 hours max +- For faster sync: Manually trigger workflow after major upstream merges + +## Configuration + +### GitHub Actions Permissions + +Required settings (already configured): + +1. **Settings ā†’ Actions ā†’ General ā†’ Workflow permissions:** + - āœ“ "Read and write permissions" + - āœ“ "Allow GitHub Actions to create and approve pull requests" + +2. **Repository Settings ā†’ Branches:** + - Consider: Branch protection rule on `master` to prevent direct pushes + - Exception: Allow `github-actions[bot]` to push + +### Adjusting Sync Schedule + +Edit `.github/workflows/sync-upstream.yml`: + +```yaml +on: + schedule: + # Current: Daily at 00:00 UTC + - cron: '0 0 * * *' + + # Examples: + # Every 6 hours: '0 */6 * * *' + # Twice daily: '0 0,12 * * *' + # Weekdays only: '0 0 * * 1-5' +``` + +**Recommendation:** Keep daily schedule to balance freshness with API usage. + +## Troubleshooting + +### Issue: Workflow not running + +**Check:** +1. Actions tab ā†’ Check if workflow is disabled +2. Settings ā†’ Actions ā†’ Ensure workflows are enabled for repository + +**Fix:** +- Enable workflow: Actions ā†’ Select workflow ā†’ "Enable workflow" + +### Issue: Permission denied on push + +**Check:** +- Settings ā†’ Actions ā†’ General ā†’ Workflow permissions + +**Fix:** +- Set to "Read and write permissions" +- Enable "Allow GitHub Actions to create and approve pull requests" + +### Issue: Merge conflicts every sync + +**Root cause:** Commits being made directly to master + +**Fix:** +1. Review `.git/hooks/` for pre-commit hooks that might auto-commit +2. Check if any automation is committing to master +3. Enforce branch protection rules +4. Educate team members on feature branch workflow + +### Issue: Sync successful but CI fails + +**This is expected** if upstream introduced breaking changes or test failures. + +**Handling:** +- Upstream tests failures are upstream's responsibility +- Focus: Ensure mirror stays in sync +- Separate: Your feature branches should pass CI + +## Cost and Usage + +### GitHub Actions Minutes + +- **Sync workflow:** ~2-3 minutes per run +- **Frequency:** Daily = 60-90 minutes/month +- **Free tier:** 2,000 minutes/month (public repos: unlimited) +- **Cost:** $0 (well within limits) + +### Network Usage + +- Fetches only new commits (incremental) +- Typical: <10 MB per sync +- Total: <300 MB/month + +## Security Considerations + +### Secrets + +- Uses `GITHUB_TOKEN` (automatically provided, scoped to repository) +- No additional secrets required +- Token permissions: Minimum necessary (contents:write, issues:write) + +### Audit Trail + +All syncs are logged: +- GitHub Actions run history (90 days retention) +- Git reflog on server +- Issue creation/closure for failures + +## Integration with Other Workflows + +### Cirrus CI + +Cirrus CI tests trigger on pushes to master: +- Sync pushes ā†’ Cirrus CI runs tests on synced commits +- This validates upstream changes against your test matrix + +### AI Code Review + +AI review workflows trigger on PRs, not master pushes: +- Sync to master does NOT trigger AI reviews +- Feature branch PRs ā†’ master do trigger AI reviews + +### Windows Builds + +Windows dependency builds trigger on master pushes: +- Sync pushes ā†’ Windows builds run +- Ensures dependencies stay compatible with latest upstream + +## Support + +### Reporting Issues + +If sync consistently fails: + +1. Check open issues with label `sync-failure` +2. Review workflow logs: Actions ā†’ Failed run ā†’ View logs +3. Create issue with: + - Workflow run URL + - Error messages from logs + - Output of `git log upstream/master..origin/master` + +### Disabling Automatic Sync + +If needed (e.g., during major refactoring): + +```bash +# Disable via GitHub UI +# Actions ā†’ "Sync from Upstream (Automatic)" ā†’ "..." ā†’ Disable workflow + +# Or delete/rename the workflow file +git mv .github/workflows/sync-upstream.yml .github/workflows/sync-upstream.yml.disabled +git commit -m "Temporarily disable automatic sync" +git push +``` + +**Remember to re-enable** once work is complete. + +## References + +- Upstream repository: https://github.com/postgres/postgres +- GitHub Actions docs: https://docs.github.com/en/actions +- Git branching strategies: https://git-scm.com/book/en/v2/Git-Branching-Branching-Workflows diff --git a/.github/workflows/sync-upstream-manual.yml b/.github/workflows/sync-upstream-manual.yml new file mode 100644 index 0000000000000..362c119a128e7 --- /dev/null +++ b/.github/workflows/sync-upstream-manual.yml @@ -0,0 +1,249 @@ +name: Sync from Upstream (Manual) + +on: + workflow_dispatch: + inputs: + force_push: + description: 'Use --force-with-lease when pushing' + required: false + type: boolean + default: true + +jobs: + sync: + runs-on: ubuntu-latest + permissions: + contents: write + issues: write + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + token: ${{ secrets.GITHUB_TOKEN }} + + - name: Configure Git + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + - name: Add upstream remote + run: | + git remote add upstream https://github.com/postgres/postgres.git || true + git remote -v + + - name: Fetch upstream + run: | + echo "Fetching from upstream postgres/postgres..." + git fetch upstream master + echo "Current local master:" + git log origin/master --oneline -5 + echo "Upstream master:" + git log upstream/master --oneline -5 + + - name: Check for local commits + id: check_commits + run: | + git checkout master + LOCAL_COMMITS=$(git rev-list origin/master..upstream/master --count) + DIVERGED=$(git rev-list upstream/master..origin/master --count) + echo "commits_behind=$LOCAL_COMMITS" >> $GITHUB_OUTPUT + echo "commits_ahead=$DIVERGED" >> $GITHUB_OUTPUT + echo "Mirror is $DIVERGED commits ahead and $LOCAL_COMMITS commits behind upstream" + + if [ "$DIVERGED" -gt 0 ]; then + # Check commit messages for "dev setup" or "dev v" pattern + DEV_SETUP_COMMITS=$(git log --format=%s upstream/master...origin/master | grep -iE "^dev (setup|v[0-9])" | wc -l) + echo "dev_setup_commits=$DEV_SETUP_COMMITS" >> $GITHUB_OUTPUT + + # Check if diverged commits only touch .github/ directory + NON_GITHUB_CHANGES=$(git diff --name-only upstream/master...origin/master | grep -v "^\.github/" | wc -l) + echo "non_github_changes=$NON_GITHUB_CHANGES" >> $GITHUB_OUTPUT + + if [ "$NON_GITHUB_CHANGES" -eq 0 ]; then + echo "āœ“ All local commits are CI/CD configuration (.github/ only)" + elif [ "$DEV_SETUP_COMMITS" -gt 0 ]; then + echo "āœ“ Found $DEV_SETUP_COMMITS 'dev setup/version' commit(s)" + else + echo "āš ļø WARNING: Local commits modify files outside .github/ and are not 'dev setup/version' commits!" + git diff --name-only upstream/master...origin/master | grep -v "^\.github/" || true + fi + else + echo "non_github_changes=0" >> $GITHUB_OUTPUT + echo "dev_setup_commits=0" >> $GITHUB_OUTPUT + fi + + - name: Attempt merge + id: merge + run: | + COMMITS_AHEAD=${{ steps.check_commits.outputs.commits_ahead }} + COMMITS_BEHIND=${{ steps.check_commits.outputs.commits_behind }} + NON_GITHUB_CHANGES=${{ steps.check_commits.outputs.non_github_changes }} + DEV_SETUP_COMMITS=${{ steps.check_commits.outputs.dev_setup_commits }} + + # Check if there are problematic local commits + # Allow commits if: + # 1. Only .github/ changes (CI/CD config) + # 2. Has "dev setup/version" commits (personal development environment) + if [ "$COMMITS_AHEAD" -gt 0 ] && [ "$NON_GITHUB_CHANGES" -gt 0 ]; then + if [ "$DEV_SETUP_COMMITS" -eq 0 ]; then + echo "āŒ Local master has commits outside .github/ that are not 'dev setup/version' commits!" + echo "merge_status=conflict" >> $GITHUB_OUTPUT + exit 1 + else + echo "āœ“ Non-.github/ changes are from 'dev setup/version' commits - allowed" + fi + fi + + # Already up to date + if [ "$COMMITS_BEHIND" -eq 0 ]; then + echo "āœ“ Already up to date with upstream" + echo "merge_status=uptodate" >> $GITHUB_OUTPUT + exit 0 + fi + + # Try fast-forward first (clean case) + if [ "$COMMITS_AHEAD" -eq 0 ]; then + echo "Fast-forwarding to upstream (no local commits)..." + git merge --ff-only upstream/master + echo "merge_status=success" >> $GITHUB_OUTPUT + exit 0 + fi + + # Local commits exist (.github/ and/or dev setup/version) - rebase onto upstream + if [ "$DEV_SETUP_COMMITS" -gt 0 ]; then + echo "Rebasing local CI/CD and dev setup/version commits onto upstream..." + else + echo "Rebasing local CI/CD commits (.github/ only) onto upstream..." + fi + + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + if git rebase upstream/master; then + echo "āœ“ Successfully rebased local commits onto upstream" + echo "merge_status=success" >> $GITHUB_OUTPUT + else + echo "āŒ Rebase conflict occurred" + echo "merge_status=conflict" >> $GITHUB_OUTPUT + + # Abort the failed rebase to clean up state + git rebase --abort + exit 1 + fi + continue-on-error: true + + - name: Push to origin + if: steps.merge.outputs.merge_status == 'success' + run: | + if [ "${{ inputs.force_push }}" == "true" ]; then + git push origin master --force-with-lease + else + git push origin master + fi + echo "āœ“ Successfully synced master with upstream" + + - name: Create issue on failure + if: steps.merge.outputs.merge_status == 'conflict' + uses: actions/github-script@v7 + with: + script: | + const title = 'šŸšØ Upstream Sync Failed - Manual Intervention Required'; + const body = `## Sync Failure Report + + The automated sync from \`postgres/postgres\` failed due to conflicting commits. + + **Details:** + - Local master has ${{ steps.check_commits.outputs.commits_ahead }} commit(s) not in upstream + - Upstream has ${{ steps.check_commits.outputs.commits_behind }} new commit(s) + - Non-.github/ changes: ${{ steps.check_commits.outputs.non_github_changes }} files + + **This indicates commits were made directly to master outside .github/**, which violates the pristine mirror policy. + + **Note:** Commits to .github/ (CI/CD configuration) are allowed and will be preserved during sync. + + ### Resolution Steps: + + 1. Identify the conflicting commits: + \`\`\`bash + git fetch origin + git fetch upstream https://github.com/postgres/postgres.git master + git log upstream/master..origin/master + \`\`\` + + 2. If these commits should be preserved: + - Create a feature branch: \`git checkout -b recovery/master-commits origin/master\` + - Reset master: \`git checkout master && git reset --hard upstream/master\` + - Push: \`git push origin master --force\` + - Cherry-pick or rebase the feature branch + + 3. If these commits should be discarded: + - Reset master: \`git checkout master && git reset --hard upstream/master\` + - Push: \`git push origin master --force\` + + 4. Close this issue once resolved + + **Workflow run:** ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + `; + + // Check if issue already exists + const issues = await github.rest.issues.listForRepo({ + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + labels: 'sync-failure' + }); + + if (issues.data.length === 0) { + await github.rest.issues.create({ + owner: context.repo.owner, + repo: context.repo.repo, + title: title, + body: body, + labels: ['sync-failure', 'automation'] + }); + } + + - name: Close existing sync-failure issues + if: steps.merge.outputs.merge_status == 'success' + uses: actions/github-script@v7 + with: + script: | + const issues = await github.rest.issues.listForRepo({ + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + labels: 'sync-failure' + }); + + for (const issue of issues.data) { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issue.number, + body: 'āœ“ Sync successful - closing this issue automatically.' + }); + + await github.rest.issues.update({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issue.number, + state: 'closed' + }); + } + + - name: Summary + if: always() + run: | + echo "### Sync Summary" >> $GITHUB_STEP_SUMMARY + echo "- **Status:** ${{ steps.merge.outputs.merge_status }}" >> $GITHUB_STEP_SUMMARY + echo "- **Commits behind:** ${{ steps.check_commits.outputs.commits_behind }}" >> $GITHUB_STEP_SUMMARY + echo "- **Commits ahead:** ${{ steps.check_commits.outputs.commits_ahead }}" >> $GITHUB_STEP_SUMMARY + if [ "${{ steps.merge.outputs.merge_status }}" == "success" ]; then + echo "- **Result:** āœ“ Successfully synced with upstream" >> $GITHUB_STEP_SUMMARY + elif [ "${{ steps.merge.outputs.merge_status }}" == "uptodate" ]; then + echo "- **Result:** āœ“ Already up to date" >> $GITHUB_STEP_SUMMARY + else + echo "- **Result:** āš ļø Sync failed - manual intervention required" >> $GITHUB_STEP_SUMMARY + fi diff --git a/.github/workflows/sync-upstream.yml b/.github/workflows/sync-upstream.yml new file mode 100644 index 0000000000000..b3a6466980b0d --- /dev/null +++ b/.github/workflows/sync-upstream.yml @@ -0,0 +1,256 @@ +name: Sync from Upstream (Automatic) + +on: + schedule: + # Run hourly every day + - cron: '0 * * * *' + workflow_dispatch: + +jobs: + sync: + runs-on: ubuntu-latest + permissions: + contents: write + issues: write + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + token: ${{ secrets.GITHUB_TOKEN }} + + - name: Configure Git + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + - name: Add upstream remote + run: | + git remote add upstream https://github.com/postgres/postgres.git || true + git remote -v + + - name: Fetch upstream + run: | + echo "Fetching from upstream postgres/postgres..." + git fetch upstream master + + - name: Check for local commits + id: check_commits + run: | + git checkout master + LOCAL_COMMITS=$(git rev-list origin/master..upstream/master --count) + DIVERGED=$(git rev-list upstream/master..origin/master --count) + echo "commits_behind=$LOCAL_COMMITS" >> $GITHUB_OUTPUT + echo "commits_ahead=$DIVERGED" >> $GITHUB_OUTPUT + + if [ "$LOCAL_COMMITS" -eq 0 ]; then + echo "āœ“ Already up to date with upstream" + else + echo "Mirror is $LOCAL_COMMITS commits behind upstream" + fi + + if [ "$DIVERGED" -gt 0 ]; then + echo "āš ļø Local master has $DIVERGED commits not in upstream" + + # Check commit messages for "dev setup" or "dev v" pattern + DEV_SETUP_COMMITS=$(git log --format=%s upstream/master..origin/master | grep -iE "^dev (setup|v[0-9])" | wc -l) + echo "dev_setup_commits=$DEV_SETUP_COMMITS" >> $GITHUB_OUTPUT + + # Check if diverged commits only touch .github/ directory + NON_GITHUB_CHANGES=$(git diff --name-only upstream/master...origin/master | grep -v "^\.github/" | wc -l) + echo "non_github_changes=$NON_GITHUB_CHANGES" >> $GITHUB_OUTPUT + + if [ "$NON_GITHUB_CHANGES" -eq 0 ]; then + echo "āœ“ All local commits are CI/CD configuration (.github/ only) - will merge" + elif [ "$DEV_SETUP_COMMITS" -gt 0 ]; then + echo "āœ“ Found $DEV_SETUP_COMMITS 'dev setup/version' commit(s)" + else + echo "āš ļø WARNING: Local commits modify files outside .github/ and are not 'dev setup/version' commits!" + git diff --name-only upstream/master...origin/master | grep -v "^\.github/" || true + echo "Non-dev commits:" + git log --format=" %h %s" upstream/master..origin/master | grep -ivE "^ [a-f0-9]* dev (setup|v[0-9])" || true + fi + else + echo "non_github_changes=0" >> $GITHUB_OUTPUT + echo "dev_setup_commits=0" >> $GITHUB_OUTPUT + fi + + - name: Attempt merge + id: merge + run: | + COMMITS_AHEAD=${{ steps.check_commits.outputs.commits_ahead }} + COMMITS_BEHIND=${{ steps.check_commits.outputs.commits_behind }} + NON_GITHUB_CHANGES=${{ steps.check_commits.outputs.non_github_changes }} + DEV_SETUP_COMMITS=${{ steps.check_commits.outputs.dev_setup_commits }} + + # Check if there are problematic local commits + # Allow commits if: + # 1. Only .github/ changes (CI/CD config) + # 2. Has "dev setup/version" commits (personal development environment) + if [ "$COMMITS_AHEAD" -gt 0 ] && [ "$NON_GITHUB_CHANGES" -gt 0 ]; then + if [ "$DEV_SETUP_COMMITS" -eq 0 ]; then + echo "āŒ Local master has commits outside .github/ that are not 'dev setup/version' commits!" + echo "merge_status=conflict" >> $GITHUB_OUTPUT + exit 1 + else + echo "āœ“ Non-.github/ changes are from 'dev setup/version' commits - allowed" + fi + fi + + # Already up to date + if [ "$COMMITS_BEHIND" -eq 0 ]; then + echo "āœ“ Already up to date with upstream" + echo "merge_status=uptodate" >> $GITHUB_OUTPUT + exit 0 + fi + + # Try fast-forward first (clean case) + if [ "$COMMITS_AHEAD" -eq 0 ]; then + echo "Fast-forwarding to upstream (no local commits)..." + git merge --ff-only upstream/master + echo "merge_status=success" >> $GITHUB_OUTPUT + exit 0 + fi + + # Local commits exist (.github/ and/or dev setup/version) - rebase onto upstream + if [ "$DEV_SETUP_COMMITS" -gt 0 ]; then + echo "Rebasing local CI/CD and dev setup/version commits onto upstream..." + else + echo "Rebasing local CI/CD commits (.github/ only) onto upstream..." + fi + + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + if git rebase upstream/master; then + echo "āœ“ Successfully rebased local commits onto upstream" + echo "merge_status=success" >> $GITHUB_OUTPUT + else + echo "āŒ Rebase conflict occurred" + echo "merge_status=conflict" >> $GITHUB_OUTPUT + + # Abort the failed rebase to clean up state + git rebase --abort + exit 1 + fi + continue-on-error: true + + - name: Push to origin + if: steps.merge.outputs.merge_status == 'success' + run: | + git push origin master --force-with-lease + + COMMITS_SYNCED="${{ steps.check_commits.outputs.commits_behind }}" + echo "āœ“ Successfully synced $COMMITS_SYNCED commits from upstream" + + - name: Create issue on failure + if: steps.merge.outputs.merge_status == 'conflict' + uses: actions/github-script@v7 + with: + script: | + const title = 'šŸšØ Automated Upstream Sync Failed'; + const body = `## Automatic Sync Failure + + The daily sync from \`postgres/postgres\` failed. + + **Details:** + - Local master has ${{ steps.check_commits.outputs.commits_ahead }} commit(s) not in upstream + - Upstream has ${{ steps.check_commits.outputs.commits_behind }} new commit(s) + - Non-.github/ changes: ${{ steps.check_commits.outputs.non_github_changes }} files + - **Run date:** ${new Date().toISOString()} + + **Root cause:** Commits were made directly to master outside of .github/, which violates the pristine mirror policy. + + **Note:** Commits to .github/ (CI/CD configuration) are allowed and will be preserved during sync. + + ### Resolution Steps: + + 1. Review the conflicting commits: + \`\`\`bash + git log upstream/master..origin/master --oneline + \`\`\` + + 2. Determine if commits should be: + - **Preserved:** Create feature branch and reset master + - **Discarded:** Hard reset master to upstream + + 3. See [sync documentation](.github/docs/sync-setup.md) for detailed recovery procedures + + 4. Run manual sync workflow after resolution to verify + + **Workflow run:** ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + `; + + // Check if issue already exists + const issues = await github.rest.issues.listForRepo({ + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + labels: 'sync-failure' + }); + + if (issues.data.length === 0) { + await github.rest.issues.create({ + owner: context.repo.owner, + repo: context.repo.repo, + title: title, + body: body, + labels: ['sync-failure', 'automation', 'urgent'] + }); + } else { + // Update existing issue + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issues.data[0].number, + body: `Sync failed again on ${new Date().toISOString()}\n\nWorkflow: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}` + }); + } + + - name: Close sync-failure issues + if: steps.merge.outputs.merge_status == 'success' + uses: actions/github-script@v7 + with: + script: | + const issues = await github.rest.issues.listForRepo({ + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + labels: 'sync-failure' + }); + + for (const issue of issues.data) { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issue.number, + body: `āœ“ Automatic sync successful on ${new Date().toISOString()} - synced ${{ steps.check_commits.outputs.commits_behind }} commits.\n\nClosing issue automatically.` + }); + + await github.rest.issues.update({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issue.number, + state: 'closed' + }); + } + + - name: Summary + if: always() + run: | + echo "### Daily Sync Summary" >> $GITHUB_STEP_SUMMARY + echo "- **Date:** $(date -u)" >> $GITHUB_STEP_SUMMARY + echo "- **Status:** ${{ steps.merge.outputs.merge_status }}" >> $GITHUB_STEP_SUMMARY + echo "- **Commits synced:** ${{ steps.check_commits.outputs.commits_behind }}" >> $GITHUB_STEP_SUMMARY + + if [ "${{ steps.merge.outputs.merge_status }}" == "success" ]; then + echo "" >> $GITHUB_STEP_SUMMARY + echo "āœ“ Mirror successfully updated with upstream postgres/postgres" >> $GITHUB_STEP_SUMMARY + elif [ "${{ steps.merge.outputs.merge_status }}" == "uptodate" ]; then + echo "" >> $GITHUB_STEP_SUMMARY + echo "āœ“ Mirror already up to date" >> $GITHUB_STEP_SUMMARY + else + echo "" >> $GITHUB_STEP_SUMMARY + echo "āš ļø Sync failed - check created issue for details" >> $GITHUB_STEP_SUMMARY + fi From 0d1da5f06d67594ea271fe289d049ce84a33edc8 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 13 Jul 2026 09:08:46 -0400 Subject: [PATCH 2/8] ci: AI/LLM PR review (OCR via Bedrock + Agora MCP history) The Open Code Review system: ocr-review and ocr-model-check workflows plus .github/ocr config (LiteLLM->Bedrock Claude Opus 4.8, rule.json, context.md, pg-history.py). --- .github/docs/ai-review-guide.md | 512 ++++++++++++++++++++++++++ .github/docs/bedrock-setup.md | 298 +++++++++++++++ .github/docs/cost-optimization.md | 219 +++++++++++ .github/ocr/context.md | 126 +++++++ .github/ocr/litellm.yaml | 41 +++ .github/ocr/pg-history.py | 225 +++++++++++ .github/ocr/rule.json | 65 ++++ .github/workflows/ocr-model-check.yml | 89 +++++ .github/workflows/ocr-review.yml | 427 +++++++++++++++++++++ 9 files changed, 2002 insertions(+) create mode 100644 .github/docs/ai-review-guide.md create mode 100644 .github/docs/bedrock-setup.md create mode 100644 .github/docs/cost-optimization.md create mode 100644 .github/ocr/context.md create mode 100644 .github/ocr/litellm.yaml create mode 100644 .github/ocr/pg-history.py create mode 100644 .github/ocr/rule.json create mode 100644 .github/workflows/ocr-model-check.yml create mode 100644 .github/workflows/ocr-review.yml diff --git a/.github/docs/ai-review-guide.md b/.github/docs/ai-review-guide.md new file mode 100644 index 0000000000000..eff0ed10cba4f --- /dev/null +++ b/.github/docs/ai-review-guide.md @@ -0,0 +1,512 @@ +# AI-Powered Code Review Guide + +## Overview + +This system uses Claude AI (Anthropic) to provide PostgreSQL-aware code reviews on pull requests. Reviews are similar in style to feedback from the PostgreSQL Hackers mailing list. + +## How It Works + +``` +PR Event (opened/updated) + ā†“ +GitHub Actions Workflow Starts + ā†“ +Fetch PR diff + metadata + ā†“ +Filter reviewable files (.c, .h, .sql, docs, Makefiles) + ā†“ +Route each file to appropriate review prompt + ā†“ +Send to Claude API with PostgreSQL context + ā†“ +Parse response for issues + ā†“ +Post inline comments + summary to PR + ā†“ +Add labels (security-concern, performance, etc.) +``` + +## Features + +### PostgreSQL-Specific Reviews + +**C Code Review:** +- Memory management (palloc/pfree, memory contexts) +- Concurrency (lock ordering, race conditions) +- Error handling (elog/ereport patterns) +- Performance (algorithm complexity, cache efficiency) +- Security (buffer overflows, SQL injection vectors) +- PostgreSQL conventions (naming, comments, style) + +**SQL Review:** +- PostgreSQL SQL dialect correctness +- Regression test patterns +- Performance (index usage, join strategy) +- Deterministic output for tests +- Edge case coverage + +**Documentation Review:** +- Technical accuracy +- SGML/DocBook format +- PostgreSQL style guide compliance +- Examples and cross-references + +**Build System Review:** +- Makefile correctness (GNU Make, PGXS) +- Meson build consistency +- Cross-platform portability +- VPATH build support + +### Automatic Labeling + +Reviews automatically add labels based on findings: + +- `security-concern` - Security issues, vulnerabilities +- `performance-concern` - Performance problems +- `needs-tests` - Missing test coverage +- `needs-docs` - Missing documentation +- `memory-management` - Memory leaks, context issues +- `concurrency-issue` - Deadlocks, race conditions + +### Cost Management + +- **Per-PR limit:** $15 (configurable) +- **Monthly limit:** $200 (configurable) +- **Alert threshold:** $150 +- **Skip draft PRs** to save costs +- **Skip large files** (>5000 lines) +- **Skip binary/generated files** + +## Setup + +### 1. Install Dependencies + +```bash +cd .github/scripts/ai-review +npm install +``` + +### 2. Configure API Key + +Get API key from: https://console.anthropic.com/ + +Add to repository secrets: +1. Settings ā†’ Secrets and variables ā†’ Actions +2. New repository secret +3. Name: `ANTHROPIC_API_KEY` +4. Value: Your API key +5. Add secret + +### 3. Enable Workflow + +The workflow is triggered automatically on PR events: +- PR opened +- PR synchronized (updated) +- PR reopened +- PR marked ready for review (draft ā†’ ready) + +**Draft PRs are skipped** to save costs. + +## Configuration + +### Main Configuration: `config.json` + +```json +{ + "model": "claude-3-5-sonnet-20241022", + "max_tokens_per_request": 4096, + "max_file_size_lines": 5000, + + "cost_limits": { + "max_per_pr_dollars": 15.0, + "max_per_month_dollars": 200.0, + "alert_threshold_dollars": 150.0 + }, + + "skip_paths": [ + "*.png", "*.jpg", "*.svg", + "src/test/regress/expected/*", + "*.po", "*.pot" + ], + + "auto_labels": { + "security-concern": ["security issue", "vulnerability"], + "performance-concern": ["inefficient", "O(nĀ²)"], + "needs-tests": ["missing test", "no test coverage"] + } +} +``` + +**Tunable parameters:** +- `max_tokens_per_request`: Response length (4096 = ~3000 words) +- `max_file_size_lines`: Skip files larger than this +- `cost_limits`: Adjust budget caps +- `skip_paths`: Add more patterns to skip +- `auto_labels`: Customize label keywords + +### Review Prompts + +Located in `.github/scripts/ai-review/prompts/`: + +- `c-code.md` - PostgreSQL C code review +- `sql.md` - SQL and regression test review +- `documentation.md` - Documentation review +- `build-system.md` - Makefile/Meson review + +**Customization:** Edit prompts to adjust review focus and style. + +## Usage + +### Automatic Reviews + +Reviews run automatically on PRs to `master` and `feature/**` branches. + +**Typical workflow:** +1. Create feature branch +2. Make changes +3. Push branch: `git push origin feature/my-feature` +4. Create PR +5. AI review runs automatically +6. Review AI feedback +7. Make updates if needed +8. Push updates ā†’ AI re-reviews + +### Manual Reviews + +Trigger manually via GitHub Actions: + +**Via UI:** +1. Actions ā†’ "AI Code Review" +2. Run workflow +3. Enter PR number +4. Run workflow + +**Via CLI:** +```bash +gh workflow run ai-code-review.yml -f pr_number=123 +``` + +### Interpreting Reviews + +**Inline comments:** +- Posted on specific lines of code +- Format: `**[Category]**` followed by description +- Categories: Memory, Security, Performance, etc. + +**Summary comment:** +- Posted at PR level +- Overview of files reviewed +- Issue count by category +- Cost information + +**Labels:** +- Automatically added based on findings +- Filter PRs by label to prioritize +- Remove label manually if false positive + +### Best Practices + +**Trust but verify:** +- AI reviews are helpful but not infallible +- False positives happen (~5% rate) +- Use judgment - AI doesn't have full context +- Especially verify: security and correctness issues + +**Iterative improvement:** +- AI learns from the prompts, not from feedback +- If AI consistently misses something, update prompts +- Share false positives/negatives to improve system + +**Cost consciousness:** +- Keep PRs focused (fewer files = lower cost) +- Use draft PRs for work-in-progress (AI skips drafts) +- Mark PR ready when you want AI review + +## Cost Tracking + +### View Costs + +**Per-PR cost:** +- Shown in AI review summary comment +- Format: `Cost: $X.XX | Model: claude-3-5-sonnet` + +**Monthly cost:** +- Download cost logs from workflow artifacts +- Aggregate to calculate monthly total + +**Download cost logs:** +```bash +# List recent runs +gh run list --workflow=ai-code-review.yml --limit 10 + +# Download artifact +gh run download -n ai-review-cost-log- +``` + +### Cost Estimation + +**Token costs (Claude 3.5 Sonnet):** +- Input: $0.003 per 1K tokens +- Output: $0.015 per 1K tokens + +**Typical costs:** +- Small PR (<500 lines, 5 files): $0.50-$1.00 +- Medium PR (500-2000 lines, 15 files): $1.00-$3.00 +- Large PR (2000-5000 lines, 30 files): $3.00-$7.50 + +**Expected monthly (20 PRs/month mixed sizes):** $35-50 + +### Budget Controls + +**Automatic limits:** +- Per-PR limit: Stops reviewing after $15 +- Monthly limit: Stops at $200 (requires manual override) +- Alert: Warning at $150 + +**Manual controls:** +- Disable workflow: Actions ā†’ AI Code Review ā†’ Disable +- Reduce `max_tokens_per_request` in config +- Add more patterns to `skip_paths` +- Increase `max_file_size_lines` threshold + +## Troubleshooting + +### Issue: No review posted + +**Possible causes:** +1. PR is draft (intentionally skipped) +2. No reviewable files (all binary or skipped patterns) +3. API key missing or invalid +4. Cost limit reached + +**Check:** +- Actions ā†’ "AI Code Review" ā†’ Latest run ā†’ View logs +- Look for: "Skipping draft PR" or "No reviewable files" +- Verify: `ANTHROPIC_API_KEY` secret exists + +### Issue: Review incomplete + +**Possible causes:** +1. PR cost limit reached ($15 default) +2. File too large (>5000 lines) +3. API rate limit hit + +**Check:** +- Review summary comment for "Reached PR cost limit" +- Workflow logs for "Skipping X - too large" + +**Fix:** +- Increase `max_per_pr_dollars` in config +- Increase `max_file_size_lines` (trade-off: higher cost) +- Split large PR into smaller PRs + +### Issue: False positives + +**Example:** AI flags correct code as problematic + +**Handling:** +1. Ignore the comment (human judgment overrides) +2. Reply to comment explaining why it's correct +3. If systematic: Update prompt to clarify + +**Note:** Some false positives are acceptable (5-10% rate) + +### Issue: Claude API errors + +**Error types:** +- `401 Unauthorized`: Invalid API key +- `429 Too Many Requests`: Rate limit +- `500 Internal Server Error`: Claude service issue + +**Check:** +- Workflow logs for error messages +- Claude status: https://status.anthropic.com/ + +**Fix:** +- Rotate API key if 401 +- Wait and retry if 429 or 500 +- Contact Anthropic support if persistent + +### Issue: High costs + +**Unexpected high costs:** +1. Check cost logs for large PRs +2. Review `skip_paths` - are large files being reviewed? +3. Check for repeated reviews (PR updated many times) + +**Optimization:** +- Add more skip patterns for generated files +- Lower `max_tokens_per_request` (shorter reviews) +- Increase `max_file_size_lines` to skip more files +- Batch PR updates to reduce review runs + +## Disabling AI Review + +### Temporarily disable + +**For one PR:** +- Convert to draft +- Or add `[skip ai]` to PR title (requires workflow modification) + +**For all PRs:** +```bash +# Via GitHub UI: +# Actions ā†’ "AI Code Review" ā†’ "..." ā†’ Disable workflow + +# Via git: +git mv .github/workflows/ai-code-review.yml \ + .github/workflows/ai-code-review.yml.disabled +git commit -m "Disable AI code review" +git push +``` + +### Permanently remove + +```bash +# Remove workflow +rm .github/workflows/ai-code-review.yml + +# Remove scripts +rm -rf .github/scripts/ai-review + +# Commit +git commit -am "Remove AI code review system" +git push +``` + +## Testing and Iteration + +### Shadow Mode (Week 1) + +Run reviews but don't post comments: + +1. Modify `review-pr.js`: + ```javascript + // Comment out posting functions + // await postInlineComments(...) + // await postSummaryComment(...) + ``` + +2. Reviews saved to workflow artifacts +3. Review quality offline +4. Tune prompts based on results + +### Comment Mode (Week 2) + +Post comments with `[AI Review]` prefix: + +1. Add prefix to comment body: + ```javascript + const body = `**[AI Review] [${issue.category}]**\n\n${issue.description}`; + ``` + +2. Gather feedback from developers +3. Adjust prompts and configuration + +### Full Mode (Week 3+) + +Remove prefix, enable all features: + +1. Remove `[AI Review]` prefix +2. Enable auto-labeling +3. Monitor quality and costs +4. Iterate on prompts as needed + +## Advanced Customization + +### Custom Review Prompts + +Add a new prompt for a file type: + +1. Create `.github/scripts/ai-review/prompts/my-type.md` +2. Write review guidelines (see existing prompts) +3. Update `config.json`: + ```json + "file_type_patterns": { + "my_type": ["*.ext", "special/*.files"] + } + ``` +4. Test with manual workflow trigger + +### Conditional Reviews + +Skip AI review for certain PRs: + +Modify `.github/workflows/ai-code-review.yml`: +```yaml +jobs: + ai-review: + if: | + github.event.pull_request.draft == false && + !contains(github.event.pull_request.title, '[skip ai]') && + !contains(github.event.pull_request.labels.*.name, 'no-ai-review') +``` + +### Cost Alerts + +Add cost alert notifications: + +1. Create workflow in `.github/workflows/cost-alert.yml` +2. Trigger: On schedule (weekly) +3. Aggregate cost logs +4. Post issue if over threshold + +## Security and Privacy + +### API Key Security + +- Store only in GitHub Secrets (encrypted at rest) +- Never commit to repository +- Never log in workflow output +- Rotate quarterly + +### Code Privacy + +- Code sent to Claude API (Anthropic) +- Anthropic does not train on API data +- API requests are not retained long-term +- See: https://www.anthropic.com/legal/privacy + +### Sensitive Code + +If reviewing sensitive/proprietary code: + +1. Review Anthropic's terms of service +2. Consider: Self-hosted alternative (future) +3. Or: Skip AI review for sensitive PRs (add label) + +## Support + +### Questions + +- Check this guide first +- Search GitHub issues: label:ai-review +- Check Claude API docs: https://docs.anthropic.com/ + +### Reporting Issues + +Create issue with: +- PR number +- Workflow run URL +- Error messages from logs +- Expected vs actual behavior + +### Improving Prompts + +Contributions welcome: +1. Identify systematic issue (false positive/negative) +2. Propose prompt modification +3. Test on sample PRs +4. Submit PR with updated prompt + +## References + +- Claude API: https://docs.anthropic.com/ +- Claude Models: https://www.anthropic.com/product +- PostgreSQL Hacker's Guide: https://wiki.postgresql.org/wiki/Developer_FAQ +- GitHub Actions: https://docs.github.com/en/actions + +--- + +**Version:** 1.0 +**Last Updated:** 2026-03-10 diff --git a/.github/docs/bedrock-setup.md b/.github/docs/bedrock-setup.md new file mode 100644 index 0000000000000..d8fbd898b51c6 --- /dev/null +++ b/.github/docs/bedrock-setup.md @@ -0,0 +1,298 @@ +# AWS Bedrock Setup for AI Code Review + +This guide explains how to use AWS Bedrock instead of the direct Anthropic API for AI code reviews. + +## Why Use Bedrock? + +- **AWS Credits:** Use existing AWS credits +- **Regional Availability:** Deploy in specific AWS regions +- **Compliance:** Meet specific compliance requirements +- **Integration:** Easier integration with AWS infrastructure +- **IAM Roles:** Use IAM roles instead of API keys when running on AWS + +## Prerequisites + +1. **AWS Account** with Bedrock access +2. **Bedrock Model Access** - Claude 3.5 Sonnet must be enabled +3. **IAM Permissions** for Bedrock API calls + +## Step 1: Enable Bedrock Model Access + +1. Log into AWS Console +2. Navigate to **Amazon Bedrock** +3. Go to **Model access** (left sidebar) +4. Click **Modify model access** +5. Find and enable: **Anthropic - Claude 3.5 Sonnet v2** +6. Click **Save changes** +7. Wait for status to show "Access granted" (~2-5 minutes) + +## Step 2: Create IAM User for GitHub Actions + +### Option A: IAM User with Access Keys (Recommended for GitHub Actions) + +1. Go to **IAM Console** +2. Click **Users** ā†’ **Create user** +3. Username: `github-actions-bedrock` +4. Click **Next** + +**Attach Policy:** +```json +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "bedrock:InvokeModel" + ], + "Resource": [ + "arn:aws:bedrock:*::foundation-model/anthropic.claude-3-5-sonnet-*" + ] + } + ] +} +``` + +5. Click **Create policy** ā†’ **JSON** ā†’ Paste above +6. Name: `BedrockClaudeInvokeOnly` +7. Attach policy to user +8. Click **Create user** + +**Create Access Keys:** +1. Click on the created user +2. Go to **Security credentials** tab +3. Click **Create access key** +4. Select: **Third-party service** +5. Click **Next** ā†’ **Create access key** +6. **Download** or copy: + - Access key ID (starts with `AKIA...`) + - Secret access key (only shown once!) + +### Option B: IAM Role (For AWS-hosted runners) + +If running GitHub Actions on AWS (self-hosted runners): + +1. Create IAM Role with trust policy for your EC2/ECS/EKS +2. Attach same `BedrockClaudeInvokeOnly` policy +3. Assign role to your runner infrastructure +4. No access keys needed! + +## Step 3: Configure Repository + +### A. Add AWS Secrets to GitHub + +1. Go to: **Settings** ā†’ **Secrets and variables** ā†’ **Actions** +2. Click **New repository secret** for each: + +**Secret 1:** +- Name: `AWS_ACCESS_KEY_ID` +- Value: Your access key ID from Step 2 + +**Secret 2:** +- Name: `AWS_SECRET_ACCESS_KEY` +- Value: Your secret access key from Step 2 + +**Secret 3:** +- Name: `AWS_REGION` +- Value: Your Bedrock region (e.g., `us-east-1`) + +### B. Update Configuration + +Edit `.github/scripts/ai-review/config.json`: + +```json +{ + "provider": "bedrock", + "model": "claude-3-5-sonnet-20241022", + "bedrock_model_id": "us.anthropic.claude-3-5-sonnet-20241022-v2:0", + "bedrock_region": "us-east-1", + ... +} +``` + +**Available Bedrock Model IDs:** +- US: `us.anthropic.claude-3-5-sonnet-20241022-v2:0` +- EU: `eu.anthropic.claude-3-5-sonnet-20241022-v2:0` +- Asia Pacific: `apac.anthropic.claude-3-5-sonnet-20241022-v2:0` + +**Available Regions:** +- `us-east-1` (US East - N. Virginia) +- `us-west-2` (US West - Oregon) +- `eu-central-1` (Europe - Frankfurt) +- `eu-west-1` (Europe - Ireland) +- `eu-west-2` (Europe - London) +- `ap-southeast-1` (Asia Pacific - Singapore) +- `ap-southeast-2` (Asia Pacific - Sydney) +- `ap-northeast-1` (Asia Pacific - Tokyo) + +Check current availability: https://docs.aws.amazon.com/bedrock/latest/userguide/models-regions.html + +### C. Install Dependencies + +```bash +cd .github/scripts/ai-review +npm install +``` + +This will install the AWS SDK for Bedrock. + +## Step 4: Test Bedrock Integration + +```bash +# Create test PR +git checkout -b test/bedrock-review +echo "// Bedrock test" >> test.c +git add test.c +git commit -m "Test: Bedrock AI review" +git push origin test/bedrock-review +``` + +Then create PR via GitHub UI. Check: +1. **Actions** tab - workflow should run +2. **PR comments** - AI review should appear +3. **Workflow logs** - should show "Using AWS Bedrock as provider" + +## Cost Comparison + +### Bedrock Pricing (Claude 3.5 Sonnet - us-east-1) +- Input: $0.003 per 1K tokens +- Output: $0.015 per 1K tokens + +### Direct Anthropic API Pricing +- Input: $0.003 per 1K tokens +- Output: $0.015 per 1K tokens + +**Same price!** Choose based on infrastructure preference. + +## Troubleshooting + +### Error: "Access denied to model" + +**Check:** +1. Model access enabled in Bedrock console? +2. IAM policy includes correct model ARN? +3. Region matches between config and enabled models? + +**Fix:** +```bash +# Verify model access via AWS CLI +aws bedrock list-foundation-models --region us-east-1 --query 'modelSummaries[?contains(modelId, `claude-3-5-sonnet`)]' +``` + +### Error: "InvalidSignatureException" + +**Check:** +1. AWS_ACCESS_KEY_ID correct? +2. AWS_SECRET_ACCESS_KEY correct? +3. Secrets named exactly as shown? + +**Fix:** +- Re-create access keys +- Update GitHub secrets +- Ensure no extra spaces in secret values + +### Error: "ThrottlingException" + +**Cause:** Bedrock rate limits exceeded + +**Fix:** +1. Reduce `max_concurrent_requests` in config.json +2. Add delays between requests +3. Request quota increase via AWS Support + +### Error: "Model not found" + +**Check:** +1. `bedrock_model_id` matches your region +2. Using cross-region model ID (e.g., `us.anthropic...` in us-east-1) + +**Fix:** +Update `bedrock_model_id` in config.json to match your region: +- US regions: `us.anthropic.claude-3-5-sonnet-20241022-v2:0` +- EU regions: `eu.anthropic.claude-3-5-sonnet-20241022-v2:0` + +## Switching Between Providers + +### Switch to Bedrock + +Edit `.github/scripts/ai-review/config.json`: +```json +{ + "provider": "bedrock", + ... +} +``` + +### Switch to Direct Anthropic API + +Edit `.github/scripts/ai-review/config.json`: +```json +{ + "provider": "anthropic", + ... +} +``` + +No other changes needed! The code automatically detects the provider. + +## Advanced: Cross-Region Setup + +Deploy in multiple regions for redundancy: + +```json +{ + "provider": "bedrock", + "bedrock_regions": ["us-east-1", "us-west-2"], + "bedrock_failover": true +} +``` + +Then update `review-pr.js` to implement failover logic. + +## Security Best Practices + +1. **Least Privilege:** IAM user can only invoke Claude models +2. **Rotate Keys:** Rotate access keys quarterly +3. **Audit Logs:** Enable CloudTrail for Bedrock API calls +4. **Cost Alerts:** Set up AWS Budgets alerts +5. **Secrets:** Never commit AWS credentials to git + +## Monitoring + +### AWS CloudWatch + +Bedrock metrics available: +- `Invocations` - Number of API calls +- `InvocationLatency` - Response time +- `InvocationClientErrors` - 4xx errors +- `InvocationServerErrors` - 5xx errors + +### Cost Tracking + +```bash +# Check Bedrock costs (current month) +aws ce get-cost-and-usage \ + --time-period Start=2026-03-01,End=2026-03-31 \ + --granularity MONTHLY \ + --metrics BlendedCost \ + --filter file://filter.json + +# filter.json: +{ + "Dimensions": { + "Key": "SERVICE", + "Values": ["Amazon Bedrock"] + } +} +``` + +## References + +- AWS Bedrock Docs: https://docs.aws.amazon.com/bedrock/ +- Model Access: https://docs.aws.amazon.com/bedrock/latest/userguide/model-access.html +- Bedrock Pricing: https://aws.amazon.com/bedrock/pricing/ +- IAM Best Practices: https://docs.aws.amazon.com/IAM/latest/UserGuide/best-practices.html + +--- + +**Need help?** Check workflow logs in Actions tab or create an issue. diff --git a/.github/docs/cost-optimization.md b/.github/docs/cost-optimization.md new file mode 100644 index 0000000000000..bcfc1c47b3ed8 --- /dev/null +++ b/.github/docs/cost-optimization.md @@ -0,0 +1,219 @@ +# CI/CD Cost Optimization + +## Overview + +This document describes the cost optimization strategies used in the PostgreSQL mirror CI/CD system to minimize GitHub Actions minutes and API costs while maintaining full functionality. + +## Optimization Strategies + +### 1. Skip Builds for Pristine Commits + +**Problem:** "Dev setup" commits and .github/ configuration changes don't require expensive Windows dependency builds or comprehensive testing. + +**Solution:** The Windows Dependencies workflow includes a `check-changes` job that inspects recent commits and skips builds when all commits are: +- Messages starting with "dev setup" (case-insensitive), OR +- Only modifying files under `.github/` directory + +**Implementation:** See `.github/workflows/windows-dependencies.yml` lines 42-90 + +**Savings:** +- Avoids ~45 minutes of Windows runner time per push +- Windows runners cost 2x Linux minutes (1 minute = 2 billed minutes) +- Estimated savings: ~$8-12/month + +### 2. AI Review Only on Pull Requests + +**Problem:** AI code review is expensive and unnecessary for direct commits to master or pristine commits. + +**Solution:** The AI Code Review workflow only triggers on: +- `pull_request` events (opened, synchronized, reopened, ready_for_review) +- Manual `workflow_dispatch` for testing specific PRs +- Skips draft PRs automatically + +**Implementation:** See `.github/workflows/ai-code-review.yml` lines 3-17 + +**Savings:** +- No reviews on dev setup commits or CI/CD changes +- No reviews on draft PRs (saves ~$1-3 per draft) +- Estimated savings: ~$10-20/month + +### 3. Aggressive Caching + +**Windows Dependencies:** +- Cache key: `--win64-` +- Cache duration: GitHub's default (7 days unused, 10 GB limit) +- Cache hit rate: 80-90% for stable versions + +**Node.js Dependencies:** +- AI review scripts cache npm packages +- Cache key based on `package.json` hash +- Near 100% cache hit rate + +**Savings:** +- Reduces build time from 45 minutes to ~5 minutes on cache hit +- Estimated savings: ~$15-20/month + +### 4. Weekly Scheduled Builds + +**Problem:** GitHub Actions artifacts expire after 90 days, making cached dependencies stale. + +**Solution:** Windows Dependencies runs on a weekly schedule (Sunday 4 AM UTC) to refresh artifacts before expiration. + +**Cost:** +- Weekly builds: ~45 minutes/week Ɨ 4 weeks = 180 minutes/month +- Windows multiplier: 360 billed minutes +- Cost: ~$6/month (within budget) + +**Alternative considered:** Daily builds would cost ~$50/month (rejected) + +### 5. Sync Workflow Optimization + +**Automatic Sync:** +- Runs hourly to keep mirror current +- Very lightweight: ~2-3 minutes per run +- Cost: ~150 minutes/month = $0 (within free tier) + +**Manual Sync:** +- Only runs on explicit trigger +- Used for testing and recovery +- Cost: Negligible + +### 6. Smart Workflow Triggers + +**Path-based triggers:** +```yaml +push: + paths: + - '.github/windows/manifest.json' + - '.github/workflows/windows-dependencies.yml' +``` + +Only rebuild Windows dependencies when: +- Manifest versions change +- Workflow itself is updated +- Manual trigger or schedule + +**Branch-based triggers:** +- AI review only on PRs to master, feature/**, dev/** +- Sync only affects master branch + +## Cost Breakdown + +| Component | Monthly Cost | Notes | +|-----------|-------------|-------| +| GitHub Actions - Sync | $0 | ~150 min/month (free: 2,000 min) | +| GitHub Actions - AI Review | $0 | ~200 min/month (free: 2,000 min) | +| GitHub Actions - Windows | ~$5-8 | ~2,500 min/month with optimizations | +| Claude API (Bedrock) | $30-45 | Usage-based, ~15-20 PRs/month | +| **Total** | **~$35-53/month** | | + +**Before optimizations:** ~$75-100/month +**After optimizations:** ~$35-53/month +**Savings:** ~$40-47/month (40-47% reduction) + +## Monitoring Costs + +### GitHub Actions Usage + +Check usage in repository settings: +``` +Settings ā†’ Billing and plans ā†’ View usage +``` + +Or via CLI: +```bash +gh api repos/:owner/:repo/actions/billing/workflows --jq '.workflows' +``` + +### AWS Bedrock Usage + +Monitor Claude API costs in AWS Console: +``` +AWS Console ā†’ Bedrock ā†’ Usage ā†’ Invocation metrics +``` + +Or via cost logs in artifacts: +``` +.github/scripts/ai-review/cost-log-*.json +``` + +### Setting Alerts + +**GitHub Actions:** +- No built-in alerts +- Monitor via monthly email summaries +- Consider third-party monitoring (e.g., AWS Lambda + GitHub API) + +**AWS Bedrock:** +- Set CloudWatch billing alarms +- Recommended thresholds: + - Warning: $30/month + - Critical: $50/month +- Hard cap in code: $200/month (see `config.json`) + +## Future Optimizations + +### Potential Improvements + +1. **Conditional Testing on PRs** + - Only run full Cirrus CI suite if C code or SQL changes + - Skip for docs-only PRs + - Estimated savings: ~5-10% of testing costs + +2. **Incremental AI Review** + - On PR updates, only review changed files + - Current: Reviews entire PR on each update + - Estimated savings: ~20-30% of AI costs + +3. **Dependency Build Sampling** + - Build only changed dependencies instead of all + - Requires more sophisticated manifest diffing + - Estimated savings: ~30-40% of Windows build costs + +4. **Self-hosted Runners** + - Run Linux builds on own infrastructure + - Keep Windows runners on GitHub (licensing) + - Estimated savings: ~$10-15/month + - **Trade-off:** Maintenance overhead + +### Not Recommended + +1. **Reduce sync frequency** (hourly ā†’ daily) + - Savings: Negligible (~$0.50/month) + - Cost: Increased lag with upstream (unacceptable) + +2. **Skip Windows builds entirely** + - Savings: ~$8/month + - Cost: Lose reproducible dependency builds (defeats purpose) + +3. **Reduce AI review quality** (Claude Sonnet ā†’ Haiku) + - Savings: ~$20-25/month + - Cost: Significantly worse code review quality + +## Pristine Commit Policy + +The following commits are considered "pristine" and skip expensive builds: + +1. **Dev setup commits:** + - Message starts with "dev setup" (case-insensitive) + - Examples: "dev setup v19", "Dev Setup: Update IDE config" + - Contains: .clang-format, .idea/, .vscode/, flake.nix, etc. + +2. **CI/CD configuration commits:** + - Only modify files under `.github/` + - Examples: Workflow changes, script updates, documentation + +**Why this works:** +- Dev setup commits don't affect PostgreSQL code +- CI/CD commits are tested by running the workflows themselves +- Reduces unnecessary Windows builds by ~60-70% + +**Implementation:** See `pristine-master-policy.md` for details. + +## Questions? + +For more information: +- Pristine master policy: `.github/docs/pristine-master-policy.md` +- Sync setup: `.github/docs/sync-setup.md` +- AI review guide: `.github/docs/ai-review-guide.md` +- Windows builds: `.github/docs/windows-builds.md` diff --git a/.github/ocr/context.md b/.github/ocr/context.md new file mode 100644 index 0000000000000..c4a83b85b124e --- /dev/null +++ b/.github/ocr/context.md @@ -0,0 +1,126 @@ +# OCR review context ā€” PostgreSQL contribution standards + +You are reviewing a change to a **PostgreSQL** fork. Every PR here is destined to +become a patch posted to the **pgsql-hackers** mailing list and tracked in a +**commitfest**. Review with the combined rigor, taste, and attention to detail of +the PostgreSQL committers. This context applies to the *whole* change, on top of +the per-file rules. + +## Review discipline +- Be precise and blunt; lead with the most serious problem. No praise, no + validation of the author, no disclaimers ā€” accuracy is the only metric. +- Verify every claim against the actual diff. Confirm names, signatures, line + numbers, and APIs before asserting. Never invent behavior or cite code not in + the change. If unsure, say so, and tag each finding **high / moderate / low** + confidence. +- Judge the change on its merits regardless of how the PR frames it. A draft PR + is WIP: weight design/approach feedback over style nits. + +## Patch hygiene (top rejection reasons on -hackers) +1. **Minimal diff.** The fastest way to get a patch rejected is unrelated + changes: reformatting untouched lines, rewording unrelated comments, touching + code not required by the change. Flag any hunk not needed for the stated + purpose. After the patch, the code should read as if it had always been + written that way. +2. **Atomic, bisectable commits.** Each commit must build and pass tests on its + own ā€” a broken intermediate commit breaks `git bisect`, revert, and + cherry-pick. Flag a commit that only compiles once a later commit lands. + Prefer one focused patch, or a clearly-ordered series of + independently-committable pieces. +3. **Tests + docs are mandatory.** A user-visible change without regression/TAP + tests **and** documentation is WIP, not commit-ready. New behavior needs + tests that cover edge and error paths, not just the happy path. +4. **DRY / reuse.** Prefer existing infrastructure (`List` in `pg_list.h`, + `StringInfo`, `dynahash`/`simplehash`, `palloc`/`MemoryContext`, `foreach`) + over reinventing it. Flag copy-paste and speculative abstraction alike ā€” the + community wants minimal, targeted changes that fit the subsystem's existing + patterns. +5. **Whitespace.** No trailing whitespace; tabs (width 4) for C indentation; + `git diff --check` must be clean. Whitespace-only churn on untouched lines is + a defect. + +## Committer-owned files ā€” do NOT touch in a patch (flag if present) +These are the committer's job at push time; including them causes needless +merge conflicts and is a mistake: +- **`src/include/catalog/catversion.h`** ā€” the `CATALOG_VERSION_NO` bump is done + by the **committer** when pushing. A catversion bump in the PR is **wrong** ā€” + flag it. (This is the single most common author mistake in catalog patches.) +- **Release notes** (`doc/src/sgml/release-*.sgml`) and version strings + (`configure.ac` `AC_INIT` version, `meson.build` `version`, `PG_VERSION`). + +## Generated files ā€” never hand-edit; edit the source +Flag direct edits to generated output; point the author at the source instead: +- Catalog headers `src/include/catalog/*_d.h`, `postgres.bki`, `schemapg.h`, + `system_constraints.sql` ā†’ edit the `pg_*.dat` files. +- `src/backend/nodes/{copy,equal,out,read}funcs.c` and other + `gen_node_support.pl` output ā†’ annotate the `Node` struct in its header. +- `fmgroids.h`, `fmgrprotos.h`, `fmgrtab.c` ā†’ edit `pg_proc.dat`. +- `utils/errcodes.h` ā†’ `errcodes.txt`; wait-event headers ā†’ + `wait_event_names.txt`; `lwlocknames.h` ā†’ `lwlocknames.txt`. +- `configure` ā†’ `configure.ac`; `*.po` translations are handled separately; + generated Unicode tables come from their source scripts. + +## Portability is a hard gate +PostgreSQL runs on Linux, Windows (MSVC), macOS, the BSDs and Solaris, across +**x86_64, ARM64, RISC-V, PPC64, s390x**, both endiannesses and 32/64-bit. Any +change must be portable across all of them: +- No unaligned memory access; no dependence on `char` signedness, integer/pointer + width, endianness, or struct padding for on-disk/wire formats. +- Use `int16/int32/int64`, `Size`, and `INT64_FORMAT`/`UINT64_FORMAT` (never + `%ld` for `int64`). +- Atomics/barriers only via `port/atomics` (`pg_atomic_*`, `pg_read/write_barrier`). +- **Windows/MSVC:** any `extern` variable used from another module or an + extension needs `PGDLLIMPORT` in its header; no VLAs or compiler-specific + extensions beyond the tree's C99 baseline. + +## Backward compatibility ā€” the strongest constraint +Do not break SQL behavior, the libpq wire protocol, the logical-replication +protocol, dump/restore, `pg_upgrade`, or exported/`PGDLLIMPORT` APIs without +extraordinary justification. **ABI** matters for back-branches: changing the +size/layout of an exported struct or the signature of an exported function +breaks installed extensions. + +## Mailing-list context & etiquette +Because each PR becomes a pgsql-hackers email read by a busy, expert, opinionated +audience, also flag what reliably wastes reviewer time or draws rejection: +- A patch that **does more than one thing** or bundles unrelated cleanup ā€” split it. +- **Footguns**: easy-to-misuse APIs, silent data-loss/corruption hazards, unsafe + defaults ā€” name them explicitly. +- **Performance claims without a reproducible benchmark.** +- No reference to the **design discussion / prior -hackers thread** (Message-Id) + for a non-trivial change. +- **Do not bikeshed:** keep style nits proportionate and clearly separated from + substantive correctness findings. + +## Minimalism ā€” the "ponytail" discipline +The best code is the code you never wrote (YAGNI). Before accepting new code, +apply the ladder: (1) Does this need to exist at all? (2) Can existing +code/infrastructure already do it? (3) Is this the simplest thing that works? +Flag: speculative scaffolding and config for a path that isn't wired yet; dead +code and unused "flexibility" (fields, params, abstractions, options with no +caller); premature abstraction (a helper used exactly once); knobs/GUCs/flags +nobody asked for. Minimal, targeted changes that fit the existing patterns beat +clever or general-purpose ones. + +## Comment & identity accuracy +- Comments must describe what the code does **now**. Flag aspirational/ + future-tense comments for behavior that already shipped ("will be", "for now", + "not yet", "future", and stale "TODO/FIXME/XXX/HACK"); comments that drifted + from the code they sit above; and incomplete/trailing comments. Comments + explain **why**, not what. No commented-out code. +- **ASCII only** in source and diffs ā€” no smart quotes, em-dashes, or ellipsis + characters. + +## Commit & versioning discipline +- Conventional-commit style, imperative subject, one logical change per commit, + each commit building on its own. +- Do **not** bump version numbers or generated version stamps (including + `catversion.h`) ā€” that is the maintainer's job at commit/release time. + +Understand common list shorthand so your comments are precise and not +miscommunicated: WIP (work in progress), GUC (config variable), WAL, LSN, OID, +TOAST, FSM, TAM (table access method), RLS, DSM, 2PC, PITR, CIC (concurrent index +creation), SAOP, ABI/API, backpatch (apply to supported back-branches), HEAD +(master tip), catversion (catalog version), pgindent, buildfarm, cfbot, +`s/x/y/` (suggested text substitution), footgun, bikeshedding, POLA (principle of +least astonishment). diff --git a/.github/ocr/litellm.yaml b/.github/ocr/litellm.yaml new file mode 100644 index 0000000000000..e23cc4eee6fe2 --- /dev/null +++ b/.github/ocr/litellm.yaml @@ -0,0 +1,41 @@ +# LiteLLM proxy config ā€” bridges Open Code Review (OpenAI protocol) to AWS Bedrock. +# +# This proxy is NOT a hosted service. The ocr-review.yml workflow installs it +# (`pip install 'litellm[proxy]'`) and runs it as a background process bound to +# 127.0.0.1:4000 for the duration of a single GitHub Actions job, then it exits. +# +# Auth to Bedrock: LiteLLM uses boto3's default credential chain, which reads +# the temporary AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY / AWS_SESSION_TOKEN +# minted by the workflow's OIDC "Configure AWS credentials" step; region from +# AWS_REGION. + +model_list: + - model_name: ocr-bedrock + litellm_params: + # Set the repo variable OCR_BEDROCK_MODEL to an Opus inference-profile id + # your account has access to, e.g.: + # bedrock/converse/us.anthropic.claude-opus-4-8 + # The 'converse/' prefix uses Bedrock's Converse API, which is the most + # reliable path for Claude tool-use (what OCR relies on). + model: os.environ/OCR_BEDROCK_MODEL + aws_region_name: os.environ/AWS_REGION + + # "High effort" review. Claude Opus 4.8 on Bedrock uses *adaptive* thinking + # controlled by output_config.effort. Set it DIRECTLY here ā€” NOT via + # reasoning_effort, which LiteLLM still maps to the legacy + # thinking.type.enabled that Opus 4.8 rejects. LiteLLM forwards + # output_config into additionalModelRequestFields for Anthropic models; if + # the build doesn't recognize the effort param it is dropped with a warning + # (no error) and the model reviews at its default effort. + # Valid: low|medium|high|max|xhigh (auto-clamped to the model ceiling). + output_config: + effort: xhigh + max_tokens: 32000 + +litellm_settings: + drop_params: true # silently drop params a model doesn't support + modify_params: true # auto-fix minor request incompatibilities + request_timeout: 600 + +general_settings: + master_key: os.environ/LITELLM_MASTER_KEY diff --git a/.github/ocr/pg-history.py b/.github/ocr/pg-history.py new file mode 100644 index 0000000000000..5794f8a920bd7 --- /dev/null +++ b/.github/ocr/pg-history.py @@ -0,0 +1,225 @@ +#!/usr/bin/env python3 +""" +pg-history: tie a PR's changes to PostgreSQL git + pgsql-hackers email history. + +OCR (the code reviewer) cannot call MCP servers, so this is a separate agent: +it runs a Bedrock (Claude Opus) tool-use loop wired to the Agora MCP server at +https://pg.ddx.io/mcp, lets the model search the mailing-list archives / commit +history / commitfest data, and emits a Markdown summary linking the changes to +the relevant threads (https://pg.ddx.io/m/pgsql-hackers/). + +Env: + PG_HISTORY_MCP_URL MCP endpoint (default https://pg.ddx.io/mcp) + PG_HISTORY_MODEL Bedrock model id (e.g. us.anthropic.claude-opus-4-8) + AWS_REGION region (creds come from the OIDC step's env) + BASE_REF, HEAD_SHA PR base ref and head sha (for the git diff context) + GH_PR_TITLE PR title (optional, adds context) + PG_HISTORY_OUT output markdown path (default /tmp/pg-history.md) +Writes the markdown to PG_HISTORY_OUT; exits 0 even on soft failures (writes a note). +""" +import json, os, subprocess, sys, urllib.request + +MCP_URL = os.environ.get("PG_HISTORY_MCP_URL", "https://pg.ddx.io/mcp") +MODEL = os.environ.get("PG_HISTORY_MODEL", "us.anthropic.claude-opus-4-8").replace("bedrock/converse/", "").replace("bedrock/", "") +REGION = os.environ.get("AWS_REGION", "us-east-1") +BASE_REF = os.environ.get("BASE_REF", "") +HEAD_SHA = os.environ.get("HEAD_SHA", "") +PR_TITLE = os.environ.get("GH_PR_TITLE", "") +OUT = os.environ.get("PG_HISTORY_OUT", "/tmp/pg-history.md") +UA = "pg-history/0.1 (+github-actions)" + +# Curated subset of the 108 Agora tools ā€” the ones useful for connecting a +# change to its discussion/commit history. Intersected with what the server +# actually exposes, so unknown names are harmless. +TOOL_WHITELIST = { + "find_related_discussions", "find_similar_messages", "get_thread", + "discussion_links", "get_author_messages", "browse_by_date", + "blame_symbol", "check_upstream_status", "find_related", + "find_entries_for_thread", "find_entries_for_author", "get_commit", + "search", "hybrid_search", "get_callers", "get_callees", "find_pattern", +} +MAX_ROUNDS = 14 +TOOL_RESULT_CAP = 8000 # chars per tool result fed back to the model + + +def _mcp_post(body, sid=None): + headers = {"Content-Type": "application/json", + "Accept": "application/json, text/event-stream", "User-Agent": UA} + if sid: + headers["Mcp-Session-Id"] = sid + req = urllib.request.Request(MCP_URL, data=json.dumps(body).encode(), headers=headers, method="POST") + resp = urllib.request.urlopen(req, timeout=60) + sid_out = resp.headers.get("Mcp-Session-Id") + result = None + for line in resp.read().decode().splitlines(): + line = line.strip() + if line.startswith("data:"): + line = line[5:].strip() + if not line or line.startswith("event:"): + continue + try: + obj = json.loads(line) + except Exception: + continue + if isinstance(obj, dict) and ("result" in obj or "error" in obj): + result = obj + return result, sid_out + + +class MCP: + def __init__(self): + init, self.sid = _mcp_post({"jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": {"protocolVersion": "2025-06-18", "capabilities": {}, + "clientInfo": {"name": "pg-history", "version": "0.1"}}}) + if not init or "result" not in init: + raise RuntimeError(f"MCP initialize failed: {init}") + try: + _mcp_post({"jsonrpc": "2.0", "method": "notifications/initialized", "params": {}}, self.sid) + except Exception: + pass + self._id = 1 + + def list_tools(self): + self._id += 1 + res, _ = _mcp_post({"jsonrpc": "2.0", "id": self._id, "method": "tools/list", "params": {}}, self.sid) + return (res or {}).get("result", {}).get("tools", []) + + def call(self, name, args): + self._id += 1 + res, _ = _mcp_post({"jsonrpc": "2.0", "id": self._id, "method": "tools/call", + "params": {"name": name, "arguments": args or {}}}, self.sid) + if not res: + return "(no response)" + if "error" in res: + return f"ERROR: {json.dumps(res['error'])[:500]}" + parts = [] + for c in res.get("result", {}).get("content", []): + if c.get("type") == "text": + parts.append(c["text"]) + return ("\n".join(parts) or "(empty)")[:TOOL_RESULT_CAP] + + +def git(*args): + try: + return subprocess.check_output(["git", *args], text=True, stderr=subprocess.DEVNULL).strip() + except Exception: + return "" + + +def pr_context(): + base = f"origin/{BASE_REF}" if BASE_REF else "" + rng = f"{base}..{HEAD_SHA}" if base and HEAD_SHA else HEAD_SHA + commits = git("log", "--no-merges", "--format=%h %s", f"{rng}") if rng else "" + stat = git("diff", "--stat", rng) if rng else "" + files = git("diff", "--name-only", rng) if rng else "" + return commits[:4000], stat[:3000], files[:2000] + + +SYSTEM = """You are a PostgreSQL community research assistant. Given a pull request's +commits and changed files, use the available tools (backed by the Agora index of +pgsql-hackers mail, commit history, and commitfest data) to connect the change to +its history. Your goal: + +- Find the mailing-list thread(s) and prior discussion behind this change. +- Identify related/superseded prior commits and any commitfest entry. +- Note relevant prior art, rejected approaches, or design rationale. + +Rules (voice & rigor): +- Be precise and blunt. No praise, no filler, no hedging, no disclaimers. Accuracy is + the only success metric ā€” not the author's approval. Lead with the most important finding. +- NEVER hallucinate. Verify every Message-ID, thread subject, commit hash, author name, + and date against an actual tool result before citing it. If a search returns nothing, + say so plainly ā€” do not guess or fabricate a plausible-looking link. +- Assess the change on its merits, independent of how the PR frames it. +- Tag any inferred (not tool-confirmed) linkage with an explicit confidence level: + high / moderate / low. +- Be decisive and efficient: a handful of targeted tool calls, not exhaustive search. +- Cite every mailing-list message as a Markdown link: [subject](https://pg.ddx.io/m/pgsql-hackers/MESSAGE_ID). +- If you find nothing relevant, say so in one line ā€” do not pad. + +When done, output ONLY Markdown (no preamble) with these sections, omitting any that are empty: +## šŸ§µ Related discussion +## šŸ”— Related commits / prior art +## šŸ“‹ Commitfest +## šŸ§­ Context for reviewers +Keep it tight (use bullets; link generously).""" + + +def to_toolspec(t): + schema = t.get("inputSchema") or {"type": "object", "properties": {}} + return {"toolSpec": {"name": t["name"], + "description": (t.get("description") or "")[:600], + "inputSchema": {"json": schema}}} + + +def main(): + commits, stat, files = pr_context() + if not commits and not files: + open(OUT, "w").write("") # nothing to do + print("No PR diff context; skipping.") + return + user = (f"PR title: {PR_TITLE}\n\n" if PR_TITLE else "") + \ + f"Commits:\n{commits or '(none)'}\n\nChanged files:\n{files or '(none)'}\n\nDiffstat:\n{stat or '(none)'}\n" + + try: + mcp = MCP() + tools = [to_toolspec(t) for t in mcp.list_tools() if t.get("name") in TOOL_WHITELIST] + except Exception as e: + open(OUT, "w").write(f"_pg-history: could not reach the Agora MCP server ({MCP_URL}): {e}_\n") + print(f"MCP unavailable: {e}") + return + if not tools: + open(OUT, "w").write("_pg-history: no usable MCP tools available._\n") + return + + import boto3 + from botocore.config import Config + + # botocore's default read timeout (60s) is too short for a multi-round + # (MAX_ROUNDS) tool-use loop against a large PR diff on a reasoning model; + # each converse() call alone can take several minutes. Bump it well past + # what a single round needs; connect_timeout stays short since a stuck + # TCP handshake is a different (and much cheaper to detect) failure mode. + brt = boto3.client("bedrock-runtime", region_name=REGION, + config=Config(read_timeout=900, connect_timeout=10)) + messages = [{"role": "user", "content": [{"text": user}]}] + final_text = "" + try: + for _ in range(MAX_ROUNDS): + resp = brt.converse( + modelId=MODEL, + system=[{"text": SYSTEM}], + messages=messages, + toolConfig={"tools": tools}, + inferenceConfig={"maxTokens": 4096}, + ) + out = resp["output"]["message"] + messages.append(out) + if resp.get("stopReason") == "tool_use": + results = [] + for blk in out["content"]: + tu = blk.get("toolUse") + if not tu: + continue + res_text = mcp.call(tu["name"], tu.get("input") or {}) + results.append({"toolResult": {"toolUseId": tu["toolUseId"], + "content": [{"text": res_text}]}}) + messages.append({"role": "user", "content": results}) + continue + final_text = "".join(b.get("text", "") for b in out["content"]).strip() + break + except Exception as e: + open(OUT, "w").write(f"_pg-history: Bedrock call failed: {e}_\n") + print(f"Bedrock error: {e}") + return + + if not final_text: + final_text = "_pg-history: no related history found._" + body = "## šŸ“œ Change history & discussion (Agora / pg.ddx.io)\n\n" + final_text + \ + "\n\nGenerated by pg-history via the Agora MCP server (pg.ddx.io).\n" + open(OUT, "w").write(body) + print(body) + + +if __name__ == "__main__": + main() diff --git a/.github/ocr/rule.json b/.github/ocr/rule.json new file mode 100644 index 0000000000000..60e13e73dcbe0 --- /dev/null +++ b/.github/ocr/rule.json @@ -0,0 +1,65 @@ +{ + "_comment": "OCR per-file review rules for PostgreSQL core + extensions. Cross-cutting contribution standards & mailing-list etiquette live in .github/ocr/context.md, passed via --background-file. OCR uses FIRST-MATCH-WINS in declaration order, so rules are ordered most-specific first. merge_system_rule:true keeps OCR's built-in fine-tuned checks (thread-safety, injection, NPE) alongside these PostgreSQL-specific rules.", + "rules": [ + { + "path": "src/test/**", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL tests. Coverage is mandatory for any behavioral change and must include edge cases (NULL, empty, boundary/overflow) and ERROR paths, not just the happy path. A test that still passes with the feature reverted is worthless ā€” confirm it actually exercises and would catch regressions in the new code. Regression (.sql/expected): deterministic, portable output ā€” ORDER BY where row order matters, no timing/plan-dependent output except intentional EXPLAIN, no absolute paths, locale-independent (C collation or explicit COLLATE), DROP objects the test creates; expected/ output must stay stable across platforms and under the parallel schedule. Concurrency/locking belongs in isolation tests (src/test/isolation, .spec + permutations). End-to-end/crash/replication/CLI behavior belongs in TAP tests (t/*.pl with PostgreSQL::Test::Cluster/Utils) ā€” no hardcoded ports/paths, no sleep as synchronization (use poll_query_until/wait_for), skip cleanly when prerequisites are missing, and clean up nodes." + }, + { + "path": "**/*.{c,h}", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL backend/frontend C ā€” review as pgsql-hackers committers do, in priority order.\n\n(1) CORRECTNESS (highest): Memory ā€” every palloc lives in the right MemoryContext; error paths via ereport/elog(ERROR) must not leak memory/buffers/locks/fds (rely on MemoryContext/ResourceOwner reset or PG_TRY/PG_FINALLY); no use-after-free; delete temp contexts. Concurrency ā€” consistent lock ordering (deadlock-free), correct lock levels, balanced LWLockAcquire/Release and START_/END_CRIT_SECTION, no TOCTOU, CHECK_FOR_INTERRUPTS in long loops, async-signal-safe signal handlers (volatile sig_atomic_t). WAL ā€” any change to shared on-disk state must be WAL-logged AND correctly replayed (redo path), crash- and replica-consistent. NULL/edge/overflow handling.\n\n(2) BACKWARD COMPATIBILITY / ABI: don't break behavior, dump/restore, pg_upgrade, libpq wire protocol, logical-replication protocol, or exported/PGDLLIMPORT'd APIs (struct size/layout, function signatures) without extraordinary justification.\n\n(3) CATALOG / GENERATED: new/changed catalog data goes in pg_*.dat, NOT the generated *_d.h/.bki. New Node types: ANNOTATE the struct in its header so gen_node_support.pl regenerates copy/equal/out/read ā€” do NOT hand-edit *funcs.c. New SQL-callable functions: add to pg_proc.dat with an OID from the 8000-9999 developer range (src/include/catalog/unused_oids; check duplicate_oids); committer renumbers at commit. DO NOT bump CATALOG_VERSION_NO in the patch ā€” flag any catversion.h change as a mistake (committer's job).\n\n(4) PERFORMANCE: no regression on hot paths; avoid O(n^2) where better is feasible; minimize work under contended locks; avoid needless palloc churn and large struct copies in hot paths.\n\n(5) SECURITY: bounded string ops (snprintf/strlcpy/strlcat ā€” never strcpy/strcat/sprintf); integer/size-overflow checks before allocation; never user input as a format string; privilege checks via pg_*_aclcheck; beware search_path and SECURITY DEFINER.\n\n(6) PORTABILITY (hard gate): no unaligned access; no dependence on char signedness, int/long/pointer width, endianness, or struct padding for on-disk/wire formats; use int16/int32/int64 + INT64_FORMAT/UINT64_FORMAT (never %ld for int64); align contended shared structs (pg_attribute_aligned/cache-line pad). Atomics/barriers only via port/atomics (pg_atomic_*, pg_read/write_barrier) ā€” never raw intrinsics or volatile-as-barrier. WINDOWS/MSVC: extern vars used cross-module/extension need PGDLLIMPORT; no VLAs or features beyond the C99 baseline the tree targets; use pg_pread/pg_pwrite. Applies across x86_64/ARM64/RISC-V/PPC64/s390x, big/little endian, 32/64-bit.\n\n(7) CONVENTIONS: errmsg starts lowercase, no trailing period, no embedded newlines; errdetail/errhint are complete capitalized sentences; correct ERRCODE_*; wrap user-facing text in _(); errmsg_plural for counts. Assert() only for can't-happen invariants (never user-reachable). Naming: snake_case with subsystem prefix (heap_insert) or CamelCase for major subsystems (ExecInitNode); ALL_CAPS macros. Must pgindent cleanly (tabs, width 4). Comments explain WHY not WHAT; no #ifdef 0 blocks, no commented-out code, no #ifdef fencing your feature. Reuse existing helpers (DRY)." + }, + { + "path": "**/*.dat", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL catalog data (pg_proc.dat, pg_type.dat, etc.) ā€” the SOURCE for generated headers. The generated *_d.h, postgres.bki, fmgroids.h, fmgrtab.c must NOT be hand-edited (they regenerate from these files). OIDs: use a value from the developer range 8000-9999 (src/include/catalog/unused_oids; verify with duplicate_oids); committer renumbers to a final contiguous block, so stay in-range and unique but don't over-optimize the exact number. Keep proc entries complete/consistent (prosrc, provolatile, proparallel, prorettype/proargtypes, matching description). DO NOT bump CATALOG_VERSION_NO / catversion.h ā€” committer's job at push time; flag any such change. New catalog columns/views need documentation in doc/src/sgml/catalogs.sgml." + }, + { + "path": "**/*.{sql,pgsql}", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL SQL. Valid PostgreSQL dialect (not MySQL/Oracle); correct types (bigint vs int, text vs varchar); sound transaction/isolation and CTE-materialization assumptions. SECURITY: flag SQL injection in dynamic SQL (require quote_identifier/quote_literal or format() with %I/%L), SECURITY DEFINER without a locked-down search_path, inappropriate RLS bypass. Prefer set-based over row-at-a-time/N+1. BACKWARD COMPATIBILITY (a top rejection reason): changing existing SQL behavior, the output of existing functions, or default GUCs needs extraordinary justification. New SQL-callable objects belong in pg_*.dat with OIDs from the 8000-9999 range, not in generated files. Minimal diff; add regression tests + docs." + }, + { + "path": "**/*.{pl,pm}", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL Perl (TAP tests and build/catalog tooling). Require 'use strict; use warnings;'. Must be perltidy-clean with the tree's src/tools/pgindent/perltidyrc and pass src/tools/perlcheck/pgperlcritic. Use the framework: PostgreSQL::Test::Cluster, PostgreSQL::Test::Utils, Test::More; no hardcoded ports/paths/PIDs; use safe_psql/poll_query_until, not sleep; skippable without optional prerequisites; clean up nodes. PORTABILITY: run on Windows (no fork-only constructs, use File::Spec, avoid unavailable signals) and the minimum supported Perl. Robustness: avoid two-arg open and string system()/qx with interpolated data (use list forms). Generator scripts (gen_node_support.pl, catalog Perl) must be deterministic and stay in sync with inputs; do not commit their generated output." + }, + { + "path": "**/*.py", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL Python (build/test tooling, oauth/pytest tests, src/tools). Follow surrounding style; keep imports to the standard library unless the dependency is already required by the tree (no surprise third-party deps in build/test tooling). PORTABILITY: support the project's minimum Python 3 and run on Windows and the BSDs (use os.path/pathlib, avoid POSIX-only calls and shell=True with interpolated input). Deterministic, self-cleaning tests; no hardcoded ports/paths; skip cleanly without prerequisites. For the Perl->pytest porting effort, confirm behavior parity with the TAP test replaced (same assertions/coverage), not a superficial translation. Minimal diff; match the tree's ruff/black config if present." + }, + { + "path": "**/*.{rs,toml}", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. Rust PostgreSQL extension (pgrx) or Rust support crate. Not core C, but it runs inside/alongside the backend, so backend safety applies. SAFETY: in code reachable from an SQL call, a Rust panic aborts the Postgres process ā€” forbid unwrap()/expect()/panic!/unreachable!/todo! and index-panics on reachable paths; use Result and pgrx error reporting (error!/ereport!). Every `unsafe` block needs a comment justifying its invariant; scrutinize raw pointers and FFI across the pg_sys boundary. pgrx: honor #[pg_guard] on extern C fns (correct panic/longjmp handling); never hold Rust references across SPI or anything that can longjmp (skips Rust destructors -> leaks); respect MemoryContext lifetimes for palloc'd data; datum<->Rust conversions must handle NULL. Concurrency uses Postgres shmem/LWLocks (pgrx shmem API), not std::sync alone. Lints: must pass `cargo clippy --all-targets --all-features -- -D warnings` and `cargo fmt --check`; deny unwrap_used/expect_used/panic in libraries; thiserror (libs) / anyhow (bins). Justify every new dependency. Tests: #[pg_test] for in-backend behavior, #[test] for pure logic; cover error and NULL paths. Minimal, idiomatic diff." + }, + { + "path": "**/{configure.ac,*.m4,aclocal.m4}", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL Autoconf. Edit configure.ac / the m4 macros ā€” do NOT hand-edit generated 'configure' or pg_config.h.in in the same patch (regeneration is the committer's step; a patch that also rewrites generated configure output is suspect). Feature/header/function probes must be portable and not assume a specific OS/compiler. Every configure knob must be mirrored on the Meson side (meson_options.txt/meson.build) and documented. Minimal diff." + }, + { + "path": "**/{meson.build,meson_options.txt}", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL Meson build. Valid syntax; correct subdir()/dependency()/declare_dependency and install paths; new source files must be added here. CRITICAL: PostgreSQL maintains BOTH Meson and Autoconf/Make ā€” any new file, option, or feature check must be mirrored on the configure.ac/Makefile side so the two never drift (a file built by only one system is a common defect). New options need matching docs and sensible defaults. Minimal diff." + }, + { + "path": "**/{Makefile,GNUmakefile,*.mk}", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL Makefile (GNU Make). $(VAR) refs; correct .PHONY; accurate dependencies (no parallel -j races); $(MAKE) for recursion; VPATH/out-of-tree build support; no hardcoded paths (use standard PostgreSQL makefile vars and $(top_builddir)); clean/distclean/maintainer-clean must remove new artifacts; extensions use PGXS. Must stay in sync with meson.build. Minimal diff." + }, + { + "path": "doc/**/*.sgml", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. PostgreSQL documentation (DocBook SGML). Technically accurate/complete (parameters, limitations, version/compat notes); correct tag usage/nesting (, , , , , /); working cross-references; spell it 'PostgreSQL' in prose; SQL keywords uppercase in examples. Coverage: a new GUC -> config.sgml (and postgresql.conf.sample); new/changed catalogs or views -> catalogs.sgml; new SQL syntax -> the matching ref/*.sgml; new functions -> func.sgml. Do NOT edit release-notes (release-*.sgml) ā€” written by the release team/committers; flag such edits. New user-facing behavior in this PR should ship with matching docs." + }, + { + "path": "**/*.md", + "merge_system_rule": true, + "rule": "REVIEW DISCIPLINE: Precise, blunt, verify against the diff, tag confidence, no praise. Markdown docs. Clear heading hierarchy; fenced code blocks with language hints; accurate instructions/prerequisites; consistent PostgreSQL terminology; no broken relative links or stale claims. Minimal diff." + } + ] +} diff --git a/.github/workflows/ocr-model-check.yml b/.github/workflows/ocr-model-check.yml new file mode 100644 index 0000000000000..10d250528cf7c --- /dev/null +++ b/.github/workflows/ocr-model-check.yml @@ -0,0 +1,89 @@ +# Checks AWS Bedrock weekly for a newer Claude Opus inference profile than the +# one OCR currently uses (vars.OCR_BEDROCK_MODEL) and, if found, opens/updates a +# single GitHub issue telling the maintainer to bump the variable. It does NOT +# change the model automatically: GITHUB_TOKEN cannot write Actions *variables* +# (that needs a PAT with admin), so this is a notify-only mechanism by design. +name: OCR model self-check + +on: + schedule: + - cron: '0 12 * * 1' # Mondays 12:00 UTC + workflow_dispatch: + +permissions: + id-token: write + contents: read + issues: write + +jobs: + check-model: + runs-on: ubuntu-latest + steps: + - name: Configure AWS credentials (OIDC) + uses: aws-actions/configure-aws-credentials@v6 + with: + role-to-assume: ${{ vars.AWS_ROLE_ARN }} + aws-region: ${{ vars.AWS_REGION }} + role-session-name: ocr-model-check-${{ github.run_id }} + + - name: Find newest Opus vs configured + id: check + env: + CURRENT: ${{ vars.OCR_BEDROCK_MODEL }} + AWS_REGION: ${{ vars.AWS_REGION }} + run: | + python3 - <<'PY' >> "$GITHUB_OUTPUT" + import os, re, subprocess, json + region = os.environ.get("AWS_REGION", "us-east-1") + current = os.environ.get("CURRENT", "") + out = subprocess.run( + ["aws", "bedrock", "list-inference-profiles", "--region", region, + "--query", "inferenceProfileSummaries[].inferenceProfileId", "--output", "json"], + capture_output=True, text=True) + ids = json.loads(out.stdout or "[]") + # Parse claude-opus-- from any profile id (prefix us./global. ok). + def ver(s): + m = re.search(r"claude-opus-(\d+)-(\d+)", s) + return (int(m.group(1)), int(m.group(2))) if m else None + opus = [(ver(i), i) for i in ids if ver(i) and i.startswith(("us.", "global."))] + if not opus: + print("newer=false"); raise SystemExit(0) + best_ver, best_id = max(opus, key=lambda x: x[0]) + cur = ver(current) + newer = (cur is None) or (best_ver > cur) + print(f"newer={'true' if newer else 'false'}") + print(f"best_id={best_id}") + print(f"best_ver={best_ver[0]}.{best_ver[1]}") + print(f"cur_ver={'unknown' if cur is None else f'{cur[0]}.{cur[1]}'}") + PY + + - name: Open/update issue if a newer model exists + if: steps.check.outputs.newer == 'true' + uses: actions/github-script@v9 + with: + script: | + const best = '${{ steps.check.outputs.best_id }}'; + const bestVer = '${{ steps.check.outputs.best_ver }}'; + const curVer = '${{ steps.check.outputs.cur_ver }}'; + const marker = ''; + const title = `OCR: newer Claude Opus available (${bestVer} > ${curVer})`; + const body = `${marker}\n` + + `A newer Claude Opus inference profile is available on Bedrock.\n\n` + + `- **Configured** (\`vars.OCR_BEDROCK_MODEL\`): Opus ${curVer}\n` + + `- **Newest on Bedrock**: \`${best}\` (Opus ${bestVer})\n\n` + + `To upgrade, set the repo variable:\n\n` + + '```\n' + + `gh variable set OCR_BEDROCK_MODEL -R ${context.repo.owner}/${context.repo.repo} \\\n` + + ` -b "bedrock/converse/${best}"\n` + + '```\n\n' + + `Also confirm the \`ocr-bedrock-ci\` IAM inline policy allows invoking the new model ` + + `(the resource is scoped to \`anthropic.claude-opus-*\`), then re-run OCR.\n\n` + + `_Automated by \`.github/workflows/ocr-model-check.yml\`; this issue is upserted._`; + const q = `repo:${context.repo.owner}/${context.repo.repo} in:body "${marker}" state:open`; + const found = await github.rest.search.issuesAndPullRequests({ q, per_page: 1 }); + if (found.data.total_count > 0) { + const n = found.data.items[0].number; + await github.rest.issues.update({ owner: context.repo.owner, repo: context.repo.repo, issue_number: n, title, body }); + } else { + await github.rest.issues.create({ owner: context.repo.owner, repo: context.repo.repo, title, body }); + } diff --git a/.github/workflows/ocr-review.yml b/.github/workflows/ocr-review.yml new file mode 100644 index 0000000000000..0828af429b57c --- /dev/null +++ b/.github/workflows/ocr-review.yml @@ -0,0 +1,427 @@ +# Open Code Review (OCR) ā€” AI PR review backed by AWS Bedrock via a LiteLLM proxy. +# +# Flow: +# PR opened/updated (incl. DRAFTS) ā”€ā” +# /open-code-review PR comment ā”€ā”¼ā”€ā–ŗ start LiteLLM (127.0.0.1:4000 ā†’ Bedrock) +# manual workflow_dispatch ā”€ā”˜ ā””ā–ŗ ocr review --format json +# ā””ā–ŗ post inline PR review comments +# +# Required (repo settings ā€” all repo *variables*, no secrets; auth is via GitHub OIDC): +# vars.AWS_ROLE_ARN - IAM role to assume via OIDC (granting bedrock:InvokeModel*) +# vars.AWS_REGION - e.g. us-east-1 +# vars.OCR_BEDROCK_MODEL - LiteLLM model string for the Opus inference profile, e.g. +# bedrock/converse/us.anthropic.claude-opus-4-8 +# +# No static AWS keys are stored. GITHUB_TOKEN (auto) posts the review comments. + +name: OCR AI Review + +on: + pull_request: + # Note: no draft filter ā€” drafts are reviewed too. + types: [opened, synchronize, reopened, ready_for_review] + issue_comment: + types: [created] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to review' + required: true + type: number + +# One review per PR; cancel superseded runs to save Bedrock spend. +concurrency: + group: ocr-review-${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }} + cancel-in-progress: true + +permissions: + id-token: write # required to mint the GitHub OIDC token for AWS role assumption + contents: read + pull-requests: write + +jobs: + ocr-review: + runs-on: ubuntu-latest + # PR events always; comment events only when the comment is on a PR and + # starts with the trigger keyword; manual dispatch always. + if: | + github.event_name == 'pull_request' || + github.event_name == 'workflow_dispatch' || + (github.event_name == 'issue_comment' && github.event.issue.pull_request && + (startsWith(github.event.comment.body, '/open-code-review') || + startsWith(github.event.comment.body, '@open-code-review'))) + + env: + # LiteLLM listens on localhost only; this key never leaves the runner. + LITELLM_MASTER_KEY: sk-ocr-ci-local + OCR_BEDROCK_MODEL: ${{ vars.OCR_BEDROCK_MODEL }} + # Region is a static var (safe at job level). AWS credentials are NOT set + # here ā€” they're minted by the OIDC "Configure AWS credentials" step below + # and exported to the environment for the LiteLLM/boto3 Bedrock calls. + AWS_REGION: ${{ vars.AWS_REGION }} + + steps: + - name: Resolve PR context + id: pr + uses: actions/github-script@v9 + with: + script: | + let prNumber; + if (context.eventName === 'pull_request') { + prNumber = context.payload.pull_request.number; + } else if (context.eventName === 'issue_comment') { + prNumber = context.issue.number; + } else { + prNumber = parseInt('${{ github.event.inputs.pr_number }}', 10); + } + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + const { data: repo } = await github.rest.repos.get({ + owner: context.repo.owner, + repo: context.repo.repo, + }); + core.setOutput('number', String(prNumber)); + core.setOutput('base_ref', pr.base.ref); + core.setOutput('head_ref', pr.head.ref); + core.setOutput('head_sha', pr.head.sha); + core.setOutput('default_branch', repo.default_branch); + core.setOutput('cross_repo', String(pr.head.repo.full_name !== pr.base.repo.full_name)); + + # NOTE: do NOT checkout the PR head. OCR reads the diff and file contents + # straight from git refs (git diff , git show :path, + # git grep ), so the working tree is irrelevant ā€” but our OCR config + # lives on the default branch, not on the PR branch. We check out the repo + # (default ref), fetch the base/head objects, and materialize the config + # from origin/. + - name: Checkout + uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Prepare git refs and OCR config + env: + BASE_REF: ${{ steps.pr.outputs.base_ref }} + HEAD_REF: ${{ steps.pr.outputs.head_ref }} + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + DEFAULT_BRANCH: ${{ steps.pr.outputs.default_branch }} + run: | + git fetch --no-tags origin "+refs/heads/${DEFAULT_BRANCH}:refs/remotes/origin/${DEFAULT_BRANCH}" || true + git fetch --no-tags origin "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" || true + git fetch --no-tags origin "+refs/heads/${HEAD_REF}:refs/remotes/origin/${HEAD_REF}" || true + git fetch --no-tags origin "${HEAD_SHA}" || true + + # OCR config lives on the default branch; materialize it independently + # of whatever ref is checked out. + mkdir -p "$RUNNER_TEMP/ocr" + git show "origin/${DEFAULT_BRANCH}:.github/ocr/litellm.yaml" > "$RUNNER_TEMP/ocr/litellm.yaml" + git show "origin/${DEFAULT_BRANCH}:.github/ocr/rule.json" > "$RUNNER_TEMP/ocr/rule.json" + git show "origin/${DEFAULT_BRANCH}:.github/ocr/context.md" > "$RUNNER_TEMP/ocr/context.md" + echo "Config materialized:"; ls -l "$RUNNER_TEMP/ocr" + + - name: Setup Python + uses: actions/setup-python@v6 + with: + python-version: '3.12' + + - name: Setup Node.js + uses: actions/setup-node@v6 + with: + node-version: '20' + + - name: Install LiteLLM proxy + Open Code Review + run: | + python -m pip install --upgrade pip + # Pin LiteLLM to a main commit that supports Claude Opus 4.8 adaptive + # thinking (maps reasoning_effort -> output_config.effort, incl. xhigh). + # Not in any tagged release yet (PyPI latest 1.87.1 lacks the Opus + # normalizer). Bump this SHA once a release ships the feature. + pip install "litellm[proxy] @ git+https://github.com/BerriAI/litellm.git@5be0797d24a2f26eb2123e13788f90055a59d91d" + npm install -g @alibaba-group/open-code-review + + - name: Configure AWS credentials (OIDC) + uses: aws-actions/configure-aws-credentials@v6 + with: + role-to-assume: ${{ vars.AWS_ROLE_ARN }} + aws-region: ${{ vars.AWS_REGION }} + role-session-name: ocr-review-${{ github.run_id }} + + - name: Start LiteLLM proxy (Bedrock bridge) + run: | + if [ -z "$OCR_BEDROCK_MODEL" ]; then + echo "::error::vars.OCR_BEDROCK_MODEL is not set (e.g. bedrock/converse/us.anthropic.claude-opus-4-1-20250805-v1:0)" + exit 1 + fi + nohup litellm --config "$RUNNER_TEMP/ocr/litellm.yaml" --host 127.0.0.1 --port 4000 \ + > /tmp/litellm.log 2>&1 & + echo "Waiting for LiteLLM to become ready..." + for i in $(seq 1 60); do + if curl -sf http://127.0.0.1:4000/health/readiness >/dev/null; then + echo "LiteLLM ready."; exit 0 + fi + sleep 2 + done + echo "::error::LiteLLM did not become ready in time"; cat /tmp/litellm.log; exit 1 + + - name: Configure OCR + run: | + ocr config set llm.url http://127.0.0.1:4000/v1/chat/completions + ocr config set llm.auth_token "$LITELLM_MASTER_KEY" + ocr config set llm.model ocr-bedrock + ocr config set llm.use_anthropic false + ocr config set language English + + - name: Run OCR review + run: | + ocr review \ + --from "origin/${{ steps.pr.outputs.base_ref }}" \ + --to "${{ steps.pr.outputs.head_sha }}" \ + --rule "$RUNNER_TEMP/ocr/rule.json" \ + --background-file "$RUNNER_TEMP/ocr/context.md" \ + --concurrency 3 \ + --timeout 20 \ + --format json \ + > /tmp/ocr-result.json 2>/tmp/ocr-stderr.log || true + echo "----- OCR stdout -----"; cat /tmp/ocr-result.json || true + echo "----- OCR stderr -----"; cat /tmp/ocr-stderr.log || true + echo "----- LiteLLM log (tail) -----"; tail -n 50 /tmp/litellm.log || true + + - name: Post review to PR + uses: actions/github-script@v9 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const fs = require('fs'); + const prNumber = parseInt('${{ steps.pr.outputs.number }}', 10); + const commitSha = '${{ steps.pr.outputs.head_sha }}'; + + // Opus at high effort can emit dozens of findings. Posting them all + // one-by-one trips GitHub's SECONDARY rate limit (403 "content + // creation"), which is what made every run fail after the review + // was already generated. We (a) cap inline comments and overflow + // the rest into the summary, (b) prefer a single bulk createReview, + // and (c) throttle + back off with Retry-After on any fallback. + const MAX_INLINE = 25; + const sleep = (ms) => new Promise(r => setTimeout(r, ms)); + + async function withRetry(fn, label) { + for (let attempt = 1; attempt <= 5; attempt++) { + try { return await fn(); } + catch (e) { + const status = e.status || (e.response && e.response.status); + const h = (e.response && e.response.headers) || {}; + const isRate = status === 403 || status === 429; + if (!isRate || attempt === 5) throw e; + let waitMs = 0; + if (h['retry-after']) waitMs = parseInt(h['retry-after'], 10) * 1000; + else if (h['x-ratelimit-reset']) waitMs = parseInt(h['x-ratelimit-reset'], 10) * 1000 - Date.now(); + if (!waitMs || Number.isNaN(waitMs) || waitMs < 0) waitMs = 1000 * Math.pow(2, attempt); + waitMs = Math.min(waitMs, 60000) + 500; + core.warning(`${label}: rate-limited (status ${status}); waiting ${Math.round(waitMs / 1000)}s (attempt ${attempt}/5)`); + await sleep(waitMs); + } + } + } + + let result; + try { + result = JSON.parse(fs.readFileSync('/tmp/ocr-result.json', 'utf8')); + } catch (e) { + const stderr = (() => { try { return fs.readFileSync('/tmp/ocr-stderr.log', 'utf8').trim(); } catch { return ''; } })(); + await withRetry(() => github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, + body: `āš ļø **OCR** could not produce a review.\n\n\`\`\`\n${(stderr || e.message).slice(0, 8000)}\n\`\`\``, + }), 'error-comment'); + return; + } + + const comments = result.comments || []; + const warnings = result.warnings || []; + + const formatComment = (c) => { + let body = c.content || ''; + if (c.suggestion_code && c.existing_code) { + body += '\n\n```suggestion\n' + c.suggestion_code + (c.suggestion_code.endsWith('\n') ? '' : '\n') + '```'; + } + return body; + }; + const formatMarkdown = (c) => { + let md = `### šŸ“„ \`${c.path}\``; + if (c.start_line && c.end_line) md += ` (L${c.start_line}-L${c.end_line})`; + md += '\n\n' + (c.content || ''); + if (c.suggestion_code && c.existing_code) { + md += '\n\n
šŸ’” Suggested change\n\n'; + md += '**Before:**\n```\n' + c.existing_code + '\n```\n\n**After:**\n```\n' + c.suggestion_code + '\n```\n\n
'; + } + return md; + }; + + if (comments.length === 0) { + await withRetry(() => github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, + body: `āœ… **OCR**: ${result.message || 'No issues found.'}`, + }), 'no-issues-comment'); + return; + } + + const inlineAll = []; + const noLine = []; + for (const c of comments) { + const body = formatComment(c); + const hasLine = (c.start_line >= 1) || (c.end_line >= 1); + if (!hasLine) { noLine.push(c); continue; } + const rc = { path: c.path, body, side: 'RIGHT' }; + if (c.start_line >= 1 && c.end_line >= 1 && c.start_line !== c.end_line) { + rc.start_line = c.start_line; rc.line = c.end_line; rc.start_side = 'RIGHT'; + } else { + rc.line = c.end_line >= 1 ? c.end_line : c.start_line; + } + inlineAll.push({ rc, c }); + } + + const inline = inlineAll.slice(0, MAX_INLINE).map(x => x.rc); + const overflow = inlineAll.slice(MAX_INLINE).map(x => x.c); + + let summary = `šŸ” **OCR** found **${comments.length}** issue(s).`; + summary += `\n- ${inline.length} inline, ${noLine.length + overflow.length} in summary`; + if (overflow.length) summary += ` (inline capped at ${MAX_INLINE})`; + if (warnings.length) summary += `\n- āš ļø ${warnings.length} warning(s) during review`; + for (const c of noLine.concat(overflow)) summary += '\n\n---\n\n' + formatMarkdown(c); + + // Preferred path: ONE createReview carrying every inline comment. + try { + await withRetry(() => github.rest.pulls.createReview({ + owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber, + commit_id: commitSha, body: summary, event: 'COMMENT', comments: inline, + }), 'bulk-review'); + return; + } catch (e) { + core.warning(`bulk createReview failed (${e.status || '?'}: ${e.message}); falling back to throttled per-comment posting`); + } + + // Fallback: an invalid inline position (line not in the diff -> 422) + // rejects the whole bulk review. Post the summary, then each comment + // individually with a delay + backoff, skipping ones GitHub rejects. + let ok = 0; const failed = []; + try { + await withRetry(() => github.rest.pulls.createReview({ + owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber, + commit_id: commitSha, body: summary, event: 'COMMENT', + }), 'summary-review'); + } catch (err) { failed.push(`summary: ${err.message}`); } + + for (const rc of inline) { + try { + await withRetry(() => github.rest.pulls.createReviewComment({ + owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber, + commit_id: commitSha, path: rc.path, body: rc.body, + ...(rc.start_line ? { start_line: rc.start_line, start_side: rc.start_side } : {}), + line: rc.line, side: rc.side, + }), `comment ${rc.path}:${rc.line}`); + ok++; + } catch (inner) { + failed.push(`\`${rc.path}\` L${rc.line}: ${inner.message}`); + } + await sleep(1200); // stay under the secondary content-creation limit + } + + if (failed.length) { + await withRetry(() => github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, + body: `šŸ“Š OCR posted ${ok}/${inline.length} inline comment(s).\n\n
${failed.length} could not be posted\n\n${failed.join('\n')}\n
`, + }), 'summary-failures'); + } + + # Companion job: OCR can't call MCP, so this separate agent ties the PR's + # changes to PostgreSQL git + pgsql-hackers history via the Agora MCP server + # (pg.ddx.io) and posts a single, upserted "history & discussion" comment. + pg-history: + runs-on: ubuntu-latest + if: | + github.event_name == 'pull_request' || + github.event_name == 'workflow_dispatch' || + (github.event_name == 'issue_comment' && github.event.issue.pull_request && + (startsWith(github.event.comment.body, '/open-code-review') || + startsWith(github.event.comment.body, '@open-code-review') || + startsWith(github.event.comment.body, '/pg-history'))) + steps: + - name: Resolve PR context + id: pr + uses: actions/github-script@v9 + with: + script: | + let prNumber; + if (context.eventName === 'pull_request') prNumber = context.payload.pull_request.number; + else if (context.eventName === 'issue_comment') prNumber = context.issue.number; + else prNumber = parseInt('${{ github.event.inputs.pr_number }}', 10); + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber }); + core.setOutput('number', String(prNumber)); + core.setOutput('base_ref', pr.base.ref); + core.setOutput('head_sha', pr.head.sha); + core.setOutput('title', pr.title || ''); + + - name: Checkout + uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Make base/head refs available + env: + BASE_REF: ${{ steps.pr.outputs.base_ref }} + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + run: | + git fetch --no-tags origin "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}" || true + git fetch --no-tags origin "${HEAD_SHA}" || true + + - name: Setup Python + uses: actions/setup-python@v6 + with: + python-version: '3.12' + + - name: Configure AWS credentials (OIDC) + uses: aws-actions/configure-aws-credentials@v6 + with: + role-to-assume: ${{ vars.AWS_ROLE_ARN }} + aws-region: ${{ vars.AWS_REGION }} + role-session-name: pg-history-${{ github.run_id }} + + - name: Install deps + run: pip install boto3 + + - name: Run pg-history (Agora MCP) + env: + PG_HISTORY_MODEL: ${{ vars.OCR_BEDROCK_MODEL }} + AWS_REGION: ${{ vars.AWS_REGION }} + BASE_REF: ${{ steps.pr.outputs.base_ref }} + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + GH_PR_TITLE: ${{ steps.pr.outputs.title }} + PG_HISTORY_OUT: ${{ runner.temp }}/pg-history.md + run: | + python .github/ocr/pg-history.py || true + echo "----- output -----"; cat "${{ runner.temp }}/pg-history.md" 2>/dev/null || echo "(no output)" + + - name: Upsert PR comment + uses: actions/github-script@v9 + with: + script: | + const fs = require('fs'); + const path = process.env.RUNNER_TEMP + '/pg-history.md'; + let body = ''; + try { body = fs.readFileSync(path, 'utf8').trim(); } catch (e) {} + if (!body) { console.log('pg-history: empty output, nothing to post'); return; } + const prNumber = parseInt('${{ steps.pr.outputs.number }}', 10); + const marker = ''; + body = marker + '\n' + body; + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, per_page: 100 }); + const mine = comments.find(c => c.user.type === 'Bot' && c.body && c.body.includes(marker)); + if (mine) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, repo: context.repo.repo, comment_id: mine.id, body }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, body }); + } From 7d3d25f5289dbbead89e240da53ed4f3fe3aa130 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 27 Jul 2026 11:28:11 -0400 Subject: [PATCH 3/8] ci: use SYNC_TOKEN (workflow scope) for upstream sync push The default GITHUB_TOKEN is blocked by GitHub from pushing commits that touch .github/workflows/, which broke the auto-sync push step once the fork carried its own workflow files. Use a PAT with repo+workflow scope (SYNC_TOKEN secret), falling back to GITHUB_TOKEN when unset. --- .github/workflows/sync-upstream-manual.yml | 5 ++++- .github/workflows/sync-upstream.yml | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.github/workflows/sync-upstream-manual.yml b/.github/workflows/sync-upstream-manual.yml index 362c119a128e7..c89d7ee23f53a 100644 --- a/.github/workflows/sync-upstream-manual.yml +++ b/.github/workflows/sync-upstream-manual.yml @@ -21,7 +21,10 @@ jobs: uses: actions/checkout@v4 with: fetch-depth: 0 - token: ${{ secrets.GITHUB_TOKEN }} + # PAT with repo + workflow scope. The default GITHUB_TOKEN cannot push + # commits that touch .github/workflows/ (platform block). See SYNC_TOKEN + # repo secret. + token: ${{ secrets.SYNC_TOKEN || secrets.GITHUB_TOKEN }} - name: Configure Git run: | diff --git a/.github/workflows/sync-upstream.yml b/.github/workflows/sync-upstream.yml index b3a6466980b0d..a763dfa5f994f 100644 --- a/.github/workflows/sync-upstream.yml +++ b/.github/workflows/sync-upstream.yml @@ -18,7 +18,11 @@ jobs: uses: actions/checkout@v4 with: fetch-depth: 0 - token: ${{ secrets.GITHUB_TOKEN }} + # PAT with repo + workflow scope. The default GITHUB_TOKEN cannot push + # commits that touch .github/workflows/ (platform block). See SYNC_TOKEN + # repo secret. Falls back to GITHUB_TOKEN if unset (push will fail on + # workflow-file changes, but non-workflow syncs still work). + token: ${{ secrets.SYNC_TOKEN || secrets.GITHUB_TOKEN }} - name: Configure Git run: | From 6f69bff7cf9691bb318ada3459036086a25b9773 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 27 Jul 2026 12:06:36 -0400 Subject: [PATCH 4/8] ci: bump actions/checkout to v5 in sync workflows Silences the Node 20 deprecation warning on the auto-sync runs. --- .github/workflows/sync-upstream-manual.yml | 2 +- .github/workflows/sync-upstream.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/sync-upstream-manual.yml b/.github/workflows/sync-upstream-manual.yml index c89d7ee23f53a..3139e517f409f 100644 --- a/.github/workflows/sync-upstream-manual.yml +++ b/.github/workflows/sync-upstream-manual.yml @@ -18,7 +18,7 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v4 + uses: actions/checkout@v5 with: fetch-depth: 0 # PAT with repo + workflow scope. The default GITHUB_TOKEN cannot push diff --git a/.github/workflows/sync-upstream.yml b/.github/workflows/sync-upstream.yml index a763dfa5f994f..eca408a7a11c5 100644 --- a/.github/workflows/sync-upstream.yml +++ b/.github/workflows/sync-upstream.yml @@ -15,7 +15,7 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v4 + uses: actions/checkout@v5 with: fetch-depth: 0 # PAT with repo + workflow scope. The default GITHUB_TOKEN cannot push From a41ba48123b790cd3acc21efd8412395f81cdd05 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 13 Jul 2026 07:57:27 -0400 Subject: [PATCH 5/8] dev: local dev-setup This isn't a commit that will be submitting for review, it is purely for local developer tooling while developing this patch set. Ignore it. --- .clangd | 43 ++ .gdbinit | 156 +++++++ flake.lock | 78 ++++ flake.nix | 45 ++ glibc-no-fortify-warning.patch | 24 ++ pg-aliases.sh | 658 +++++++++++++++++++++++++++++ shell.nix | 745 +++++++++++++++++++++++++++++++++ src/test/regress/pg_regress.c | 2 +- src/tools/pgindent/pgindent | 2 +- 9 files changed, 1751 insertions(+), 2 deletions(-) create mode 100644 .clangd create mode 100644 .gdbinit create mode 100644 flake.lock create mode 100644 flake.nix create mode 100644 glibc-no-fortify-warning.patch create mode 100644 pg-aliases.sh create mode 100644 shell.nix diff --git a/.clangd b/.clangd new file mode 100644 index 0000000000000..490220f56a7c0 --- /dev/null +++ b/.clangd @@ -0,0 +1,43 @@ +Diagnostics: + MissingIncludes: None +InlayHints: + Enabled: true + ParameterNames: true + DeducedTypes: true +CompileFlags: + CompilationDatabase: build/ # Search build/ directory for compile_commands.json + Remove: [ -Werror ] + Add: + - -DDEBUG + - -DLOCAL + - -DPGDLLIMPORT= + - -DPIC + - -O2 + - -Wall + - -Wcast-function-type + - -Wconversion + - -Wdeclaration-after-statement + - -Wendif-labels + - -Werror=vla + - -Wextra + - -Wfloat-equal + - -Wformat-security + - -Wimplicit-fallthrough=3 + - -Wmissing-format-attribute + - -Wmissing-prototypes + - -Wno-format-truncation + - -Wno-sign-conversion + - -Wno-stringop-truncation + - -Wno-unused-const-variable + - -Wpointer-arith + - -Wshadow + - -Wshadow=compatible-local + - -fPIC + - -fexcess-precision=standard + - -fno-strict-aliasing + - -fvisibility=hidden + - -fwrapv + - -g + - -std=c11 + - -I. + - -I../../../../src/include diff --git a/.gdbinit b/.gdbinit new file mode 100644 index 0000000000000..854e5ecbaf69c --- /dev/null +++ b/.gdbinit @@ -0,0 +1,156 @@ +# HOT Indexed Updates ā€” GDB breakpoints for code review +# +# Usage: gdb -x .gdbinit +# Or from gdb: source .gdbinit +# +# These breakpoints cover the major code paths introduced or modified by +# the HOT indexed updates patch series. They are organized by subsystem +# to make it easy to enable/disable groups during debugging. +# +# Tip: to skip to a specific subsystem, disable all then enable selectively: +# disable breakpoints +# enable 1 2 3 # just the update-decision group + +# ========================================================================= +# 1. UPDATE DECISION ā€” heap_update() HOT/HOT-indexed/non-HOT choice +# src/backend/access/heap/heapam.c +# ========================================================================= + +# Main entry: heap_update +break heapam.c:3210 + +# HOT decision block: pure HOT vs HOT indexed vs non-HOT +# Line 4019: pure HOT (no indexed columns changed) +# Line 4024: HOT indexed path (non-catalog, some indexed columns changed) +# Line 4031: predict augmented tuple size +# Line 4033: size+space check before creating augmented tuple +break heapam.c:4019 +break heapam.c:4024 +break heapam.c:4033 + +# Set HEAP_INDEXED_UPDATED flag on new tuple before page insertion +break heapam.c:4101 + +# Restore HEAP_INDEXED_UPDATED on old tuple (only if it previously had it) +break heapam.c:4147 + +# ========================================================================= +# 2. TUPLE CREATION ā€” building the augmented tuple with embedded bitmap +# src/backend/access/heap/heapam.c +# ========================================================================= + +# Predict augmented tuple size (returns 0 if t_hoff would overflow) +break heap_hot_indexed_tuple_size + +# Create augmented tuple with embedded modified-column bitmap +break heap_hot_indexed_create_tuple + +# Serialize Bitmapset into raw bytes in tuple header +break heap_hot_indexed_serialize_bitmap + +# ========================================================================= +# 3. BITMAP UTILITIES ā€” raw bitmap operations for chain following +# src/backend/access/heap/heapam.c +# ========================================================================= + +# Compute raw bitmap byte size from natts +break heap_hot_indexed_bitmap_raw_size + +# Check if tuple header has room for bitmap between null bitmap and data +break heap_hot_indexed_has_bitmap_space + +# Read HOT indexed bitmap from tuple header (returns Bitmapset) +break heap_hot_indexed_read_bitmap + +# Fast overlap check: does tuple's raw bitmap overlap with indexed_attrs? +break heap_hot_indexed_bitmap_overlaps_raw + +# OR a tuple's raw bitmap into an accumulator buffer +break heap_hot_indexed_bitmap_or_raw + +# Check if accumulated raw bitmap overlaps with indexed_attrs +break heap_hot_indexed_accum_overlaps + +# Merge bitmaps from dead tuples into a target tuple on the page +break heap_hot_indexed_merge_bitmaps_raw + +# Deserialize raw bytes back to Bitmapset +break heap_hot_indexed_deserialize_bitmap + +# ========================================================================= +# 4. INDEX SCAN ā€” HOT chain following with stale-entry detection +# src/backend/access/heap/heapam_indexscan.c +# ========================================================================= + +# Main HOT chain search with indexed update awareness +break heap_hot_search_buffer + +# Redirect-with-data: initialize bitmap accumulator from collapsed redirect +break heapam_indexscan.c:182 + +# Accumulate bitmap from INDEXED_UPDATED tuple in chain +break heapam_indexscan.c:250 + +# Stale entry detection: accumulated bitmap overlaps this index's attrs +break heapam_indexscan.c:297 + +# ========================================================================= +# 5. INDEX SCAN SETUP ā€” indexed_attrs bitmap computation +# src/backend/access/index/indexam.c +# ========================================================================= + +# Compute indexed_attrs for HOT indexed update chain following +break indexam.c:299 + +# ========================================================================= +# 6. INDEX INSERTION ā€” skip unchanged indexes for HOT indexed updates +# src/backend/executor/execIndexing.c +# ========================================================================= + +# Entry: insert/update index tuples +break ExecInsertIndexTuples + +# Index skip decision: skip indexes whose attrs don't overlap modified set +break execIndexing.c:370 + +# ========================================================================= +# 7. PRUNING ā€” chain collapsing and redirect-with-data +# src/backend/access/heap/pruneheap.c +# ========================================================================= + +# Main prune function +break heap_page_prune_and_freeze + +# Per-chain pruning entry +break heap_prune_chain + +# Chain collapsing: collect bitmaps from dead INDEXED_UPDATED intermediates +break pruneheap.c:1802 + +# OR dead tuple bitmaps into combined bitmap +break pruneheap.c:1836 + +# Record redirect-with-data for execute phase +break pruneheap.c:1863 + +# Execute phase: apply redirect-with-data entries on the page +break pruneheap.c:1287 + +# ========================================================================= +# 8. WAL REPLAY ā€” recovery of HOT indexed updates +# src/backend/access/heap/heapam_xlog.c +# ========================================================================= + +# WAL replay for XLOG_HEAP2_INDEXED_UPDATE +break heap_xlog_indexed_update + +# ========================================================================= +# 9. WAL LOGGING ā€” writing HOT indexed update records +# src/backend/access/heap/heapam.c +# ========================================================================= + +# WAL logging for heap updates (handles indexed_update flag) +break log_heap_update + +# Serialize redirect-with-data into WAL record (pruneheap.c) +break pruneheap.c:2936 diff --git a/flake.lock b/flake.lock new file mode 100644 index 0000000000000..b8e8a1fdb750f --- /dev/null +++ b/flake.lock @@ -0,0 +1,78 @@ +{ + "nodes": { + "flake-utils": { + "inputs": { + "systems": "systems" + }, + "locked": { + "lastModified": 1731533236, + "narHash": "sha256-l0KFg5HjrsfsO/JpG+r7fRrqm12kzFHyUHqHCVpMMbI=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "11707dc2f618dd54ca8739b309ec4fc024de578b", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1767313136, + "narHash": "sha256-16KkgfdYqjaeRGBaYsNrhPRRENs0qzkQVUooNHtoy2w=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "ac62194c3917d5f474c1a844b6fd6da2db95077d", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixos-25.05", + "repo": "nixpkgs", + "type": "github" + } + }, + "nixpkgs-unstable": { + "locked": { + "lastModified": 1777270315, + "narHash": "sha256-yKB4G6cKsQsWN7M6rZGk6gkJPDNPIzT05y4qzRyCDlI=", + "owner": "nixos", + "repo": "nixpkgs", + "rev": "6368eda62c9775c38ef7f714b2555a741c20c72d", + "type": "github" + }, + "original": { + "owner": "nixos", + "ref": "nixpkgs-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, + "root": { + "inputs": { + "flake-utils": "flake-utils", + "nixpkgs": "nixpkgs", + "nixpkgs-unstable": "nixpkgs-unstable" + } + }, + "systems": { + "locked": { + "lastModified": 1681028828, + "narHash": "sha256-Vy1rq5AaRuLzOxct8nz4T6wlgyUR7zLU309k9mBC768=", + "owner": "nix-systems", + "repo": "default", + "rev": "da67096a3b9bf56a91d16901293e51ba5b49a27e", + "type": "github" + }, + "original": { + "owner": "nix-systems", + "repo": "default", + "type": "github" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix new file mode 100644 index 0000000000000..aae6d54c4c8cf --- /dev/null +++ b/flake.nix @@ -0,0 +1,45 @@ +{ + description = "PostgreSQL development environment"; + + inputs = { + nixpkgs.url = "github:NixOS/nixpkgs/nixos-25.05"; + nixpkgs-unstable.url = "github:nixos/nixpkgs/nixpkgs-unstable"; + flake-utils.url = "github:numtide/flake-utils"; + }; + + outputs = { + self, + nixpkgs, + nixpkgs-unstable, + flake-utils, + }: + flake-utils.lib.eachDefaultSystem ( + system: let + pkgs = import nixpkgs { + inherit system; + config.allowUnfree = true; + }; + pkgs-unstable = import nixpkgs-unstable { + inherit system; + config.allowUnfree = true; + }; + + shellConfig = import ./shell.nix {inherit pkgs pkgs-unstable system;}; + in { + formatter = pkgs.alejandra; + devShells = { + default = shellConfig.devShell; + gcc = shellConfig.devShell; + clang = shellConfig.clangDevShell; + gcc-musl = shellConfig.muslDevShell; + clang-musl = shellConfig.clangMuslDevShell; + }; + + packages = { + inherit (shellConfig) gdbConfig flameGraphScript pgbenchScript; + }; + + environment.localBinInPath = true; + } + ); +} diff --git a/glibc-no-fortify-warning.patch b/glibc-no-fortify-warning.patch new file mode 100644 index 0000000000000..4657a12adbcc5 --- /dev/null +++ b/glibc-no-fortify-warning.patch @@ -0,0 +1,24 @@ +From 130c231020f97e5eb878cc9fdb2bd9b186a5aa04 Mon Sep 17 00:00:00 2001 +From: Greg Burd +Date: Fri, 24 Oct 2025 11:58:24 -0400 +Subject: [PATCH] no warnings with -O0 and fortify source please + +--- + include/features.h | 1 - + 1 file changed, 1 deletion(-) + +diff --git a/include/features.h b/include/features.h +index 673c4036..a02c8a3f 100644 +--- a/include/features.h ++++ b/include/features.h +@@ -432,7 +432,6 @@ + + #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 + # if !defined __OPTIMIZE__ || __OPTIMIZE__ <= 0 +-# warning _FORTIFY_SOURCE requires compiling with optimization (-O) + # elif !__GNUC_PREREQ (4, 1) + # warning _FORTIFY_SOURCE requires GCC 4.1 or later + # elif _FORTIFY_SOURCE > 2 && (__glibc_clang_prereq (9, 0) \ +-- +2.50.1 + diff --git a/pg-aliases.sh b/pg-aliases.sh new file mode 100644 index 0000000000000..0c13adc8f903a --- /dev/null +++ b/pg-aliases.sh @@ -0,0 +1,658 @@ +# PostgreSQL Development Aliases + +# ============================================================ +# Build helpers shared by every variant. +# ============================================================ +pg_clean_for_compiler() { + local current_compiler="$(basename $CC)" + local build_dir="${1:-$PG_BUILD_DIR}" + + if [ -f "$build_dir/compile_commands.json" ]; then + local last_compiler=$(grep -o '/[^/]*/bin/[gc]cc\|/[^/]*/bin/clang' "$build_dir/compile_commands.json" | head -1 | xargs basename 2>/dev/null || echo "unknown") + + if [ "$last_compiler" != "$current_compiler" ] && [ "$last_compiler" != "unknown" ]; then + echo "Detected compiler change from $last_compiler to $current_compiler" + echo "Cleaning build directory..." + trash "$build_dir" 2>/dev/null || rm -rf "$build_dir" + mkdir -p "$build_dir" + fi + fi + + mkdir -p "$build_dir" + echo "$current_compiler" >"$build_dir/.compiler_used" +} + +# ============================================================ +# Core PostgreSQL commands (default/debug build) +# ============================================================ +alias pg-setup=' + if [ -z "$PERL_CORE_DIR" ]; then + echo "Error: Could not find perl CORE directory" >&2 + return 1 + fi + + pg_clean_for_compiler "$PG_BUILD_DIR" + + echo "=== PostgreSQL Build Configuration ===" + echo "Compiler: $CC" + echo "LLVM: $(llvm-config --version 2>/dev/null || echo disabled)" + echo "Source: $PG_SOURCE_DIR" + echo "Build: $PG_BUILD_DIR" + echo "Install: $PG_INSTALL_DIR" + echo "======================================" + + env CFLAGS="-I$PERL_CORE_DIR $CFLAGS" \ + LDFLAGS="-L$PERL_CORE_DIR -lperl $LDFLAGS" \ + meson setup $MESON_EXTRA_SETUP \ + --reconfigure \ + -Doptimization=g \ + -Ddebug=true \ + -Db_sanitize=none \ + -Db_lundef=false \ + -Dlz4=enabled \ + -Dzstd=enabled \ + -Dllvm=disabled \ + -Dplperl=enabled \ + -Dplpython=enabled \ + -Dpltcl=enabled \ + -Dlibxml=enabled \ + -Duuid=e2fs \ + -Dlibxslt=enabled \ + -Dssl=openssl \ + -Dldap=disabled \ + -Dcassert=true \ + -Dtap_tests=enabled \ + -Dinjection_points=true \ + -Ddocs_pdf=enabled \ + -Ddocs_html_style=website \ + --prefix="$PG_INSTALL_DIR" \ + "$PG_BUILD_DIR" \ + "$PG_SOURCE_DIR"' + +alias pg-compdb='compdb -p build/ list > compile_commands.json' +alias pg-build='meson compile -C "$PG_BUILD_DIR"' +alias pg-install='meson install -C "$PG_BUILD_DIR"' +alias pg-test='meson test -q --print-errorlogs -C "$PG_BUILD_DIR"' + +# Clean commands +alias pg-clean='ninja -C "$PG_BUILD_DIR" clean' +alias pg-full-clean='trash "$PG_BUILD_DIR" "$PG_INSTALL_DIR" 2>/dev/null || rm -rf "$PG_BUILD_DIR" "$PG_INSTALL_DIR"; echo "Build and install directories cleaned"' + +# Database management +alias pg-init='trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' + +alias pg-start='ulimit -c unlimited && "$PG_INSTALL_DIR/bin/postgres" -D "$PG_DATA_DIR" -k "$PG_DATA_DIR"' + +alias pg-stop='pkill -f "postgres.*-D.*$PG_DATA_DIR" || true' +alias pg-restart='pg-stop && sleep 2 && pg-start' +alias pg-status='pgrep -f "postgres.*-D.*$PG_DATA_DIR" && echo "PostgreSQL is running" || echo "PostgreSQL is not running"' + +# Client connections +alias pg-psql='"$PG_INSTALL_DIR/bin/psql" -h "$PG_DATA_DIR" postgres' +alias pg-createdb='"$PG_INSTALL_DIR/bin/createdb" -h "$PG_DATA_DIR"' +alias pg-dropdb='"$PG_INSTALL_DIR/bin/dropdb" -h "$PG_DATA_DIR"' + +# ============================================================ +# Debugger attachments +# ============================================================ +alias pg-debug-gdb='gdb -x "$GDBINIT" -x .gdbinit "$PG_INSTALL_DIR/bin/postgres"' +alias pg-debug-lldb='lldb "$PG_INSTALL_DIR/bin/postgres"' +alias pg-debug=' + if command -v gdb >/dev/null 2>&1; then + pg-debug-gdb + elif command -v lldb >/dev/null 2>&1; then + pg-debug-lldb + else + echo "No debugger available (gdb or lldb required)" + fi' + +alias pg-attach-gdb=' + PG_PID=$(pgrep -f "postgres.*-D.*$PG_DATA_DIR" | head -1) + if [ -n "$PG_PID" ]; then + echo "Attaching GDB to PostgreSQL process $PG_PID" + gdb -x "$GDBINIT" -x .gdbinit -p "$PG_PID" + else + echo "No PostgreSQL process found" + fi' + +alias pg-attach-lldb=' + PG_PID=$(pgrep -f "postgres.*-D.*$PG_DATA_DIR" | head -1) + if [ -n "$PG_PID" ]; then + echo "Attaching LLDB to PostgreSQL process $PG_PID" + lldb -p "$PG_PID" + else + echo "No PostgreSQL process found" + fi' + +alias pg-attach=' + if command -v gdb >/dev/null 2>&1; then + pg-attach-gdb + elif command -v lldb >/dev/null 2>&1; then + pg-attach-lldb + else + echo "No debugger available (gdb or lldb required)" + fi' + +# ============================================================ +# Valgrind-instrumented build and tests +# +# The valgrind build lives in a separate directory so the normal +# build stays warm. Runs use a wrapper dir that shadows `postgres` +# with a valgrind wrapper -- pg_regress finds it via PATH. +# ============================================================ +pg-build-valgrind() { + local bdir="$PG_BUILD_DIR_VALGRIND" + if [ -z "$PERL_CORE_DIR" ]; then + echo "Error: PERL_CORE_DIR is not set" >&2 + return 1 + fi + + pg_clean_for_compiler "$bdir" + + echo "=== Configuring Valgrind build in $bdir ===" + env CFLAGS="-Og -ggdb3 -fno-omit-frame-pointer -DUSE_VALGRIND -I$PERL_CORE_DIR $CFLAGS" \ + LDFLAGS="-L$PERL_CORE_DIR -lperl $LDFLAGS" \ + meson setup --reconfigure \ + -Doptimization=g \ + -Ddebug=true \ + -Dcassert=true \ + -Dtap_tests=enabled \ + -Dinjection_points=true \ + -Dllvm=disabled \ + -Dplperl=enabled -Dplpython=enabled -Dpltcl=enabled \ + -Dlz4=enabled -Dzstd=enabled \ + -Dlibxml=enabled -Dlibxslt=enabled -Dssl=openssl -Duuid=e2fs \ + -Dldap=disabled \ + --prefix="$PG_INSTALL_DIR-valgrind" \ + "$bdir" "$PG_SOURCE_DIR" || return 1 + + meson compile -C "$bdir" +} + +# Drop a wrapper directory that shadows the real binaries; `postgres` +# exec's into valgrind, everything else is a symlink. Writes to the +# supplied wrap dir and echoes its path. +_pg_make_valgrind_wrapper() { + local bindir="$1" + local wrapdir="$2" + + mkdir -p "$wrapdir" + cat >"$wrapdir/postgres" <&2 + return 1 + fi + + local tmpbin="$bdir/tmp_install$PG_INSTALL_DIR-valgrind/bin" + if [ ! -x "$tmpbin/postgres" ]; then + echo "Populating tmp_install..." + meson test -C "$bdir" tmp_install install_test_files initdb_cache >/dev/null || return 1 + fi + + local wrap + wrap=$(mktemp -d /tmp/pg-vg-wrap-XXXXXX) + _pg_make_valgrind_wrapper "$tmpbin" "$wrap" + + mkdir -p "$PG_BENCH_DIR" + echo "Valgrind logs: $PG_BENCH_DIR/valgrind-*.log" + echo "Wrapper dir: $wrap (will be removed on exit)" + echo "Expect the regress suite to take 15-45 minutes under valgrind." + + local rc=0 + (cd "$bdir" && PATH="$wrap:$PATH" meson test -t 60 --print-errorlogs regress/regress) || rc=$? + + trash "$wrap" 2>/dev/null || rm -rf "$wrap" + return "$rc" +} + +pg-valgrind-test() { + local bdir="$PG_BUILD_DIR_VALGRIND" + if [ ! -x "$bdir/src/backend/postgres" ]; then + echo "Valgrind build not found; run 'pg-build-valgrind' first." >&2 + return 1 + fi + + echo "This runs the FULL postgres test suite under valgrind." + echo "Expect many hours, and tens of GB of valgrind log output." + echo "Logs: $PG_BENCH_DIR/valgrind-*.log" + local yn + read -r -p "Continue? [y/N] " yn + case "$yn" in + y | Y | yes) ;; + *) echo "Aborted."; return 0 ;; + esac + + local tmpbin="$bdir/tmp_install$PG_INSTALL_DIR-valgrind/bin" + if [ ! -x "$tmpbin/postgres" ]; then + echo "Populating tmp_install..." + meson test -C "$bdir" tmp_install install_test_files initdb_cache >/dev/null || return 1 + fi + + local wrap + wrap=$(mktemp -d /tmp/pg-vg-wrap-XXXXXX) + _pg_make_valgrind_wrapper "$tmpbin" "$wrap" + mkdir -p "$PG_BENCH_DIR" + + local rc=0 + (cd "$bdir" && PATH="$wrap:$PATH" meson test -t 60 --print-errorlogs) || rc=$? + + trash "$wrap" 2>/dev/null || rm -rf "$wrap" + return "$rc" +} + +# ============================================================ +# AddressSanitizer / UndefinedBehaviorSanitizer build and tests +# ============================================================ +pg-build-asan() { + local bdir="$PG_BUILD_DIR_ASAN" + if [ -z "$PERL_CORE_DIR" ]; then + echo "Error: PERL_CORE_DIR is not set" >&2 + return 1 + fi + + pg_clean_for_compiler "$bdir" + + echo "=== Configuring ASan+UBSan build in $bdir ===" + env CFLAGS="-Og -ggdb3 -fno-omit-frame-pointer -fsanitize=address,undefined -fno-sanitize-recover=all -I$PERL_CORE_DIR $CFLAGS" \ + LDFLAGS="-fsanitize=address,undefined -L$PERL_CORE_DIR -lperl $LDFLAGS" \ + meson setup --reconfigure \ + -Doptimization=g \ + -Ddebug=true \ + -Dcassert=true \ + -Dtap_tests=enabled \ + -Dinjection_points=true \ + -Dllvm=disabled \ + -Dplperl=enabled -Dplpython=enabled -Dpltcl=enabled \ + -Dlz4=enabled -Dzstd=enabled \ + -Dlibxml=enabled -Dlibxslt=enabled -Dssl=openssl -Duuid=e2fs \ + -Dldap=disabled \ + --prefix="$PG_INSTALL_DIR-asan" \ + "$bdir" "$PG_SOURCE_DIR" || return 1 + + meson compile -C "$bdir" +} + +pg-asan-regress() { + local bdir="$PG_BUILD_DIR_ASAN" + if [ ! -x "$bdir/src/backend/postgres" ]; then + echo "ASan build not found; run 'pg-build-asan' first." >&2 + return 1 + fi + + # halt_on_error=0 lets regress continue past the first diagnostic so + # the whole suite runs; abort_on_error=1 makes each hit fail the test. + ASAN_OPTIONS="halt_on_error=0:abort_on_error=1:detect_leaks=0:print_summary=1:print_stacktrace=1" \ + UBSAN_OPTIONS="halt_on_error=1:abort_on_error=1:print_stacktrace=1:print_summary=1" \ + meson test -t 5 --print-errorlogs -C "$bdir" regress/regress +} + +# ============================================================ +# rr (deterministic record-and-replay) +# Requires kernel.perf_event_paranoid <= 1. rr is the single most +# effective tool for postgres bugs that reproduce intermittently. +# ============================================================ +pg-rr-check() { + if ! command -v rr >/dev/null; then + echo "rr is not installed (expected in the dev shell)." >&2 + return 1 + fi + local paranoid + paranoid=$(cat /proc/sys/kernel/perf_event_paranoid 2>/dev/null || echo 99) + if [ "$paranoid" -gt 1 ]; then + echo "rr requires kernel.perf_event_paranoid <= 1; currently $paranoid" + echo "To enable (root needed):" + echo " echo 1 | sudo tee /proc/sys/kernel/perf_event_paranoid" + return 1 + fi + echo "rr ready (perf_event_paranoid=$paranoid)" +} + +pg-rr-record() { + pg-rr-check >/dev/null || { + pg-rr-check + return 1 + } + ulimit -c unlimited + rr record -- "$PG_INSTALL_DIR/bin/postgres" -D "$PG_DATA_DIR" -k "$PG_DATA_DIR" +} + +pg-rr-replay() { + rr replay "$@" +} + +# ============================================================ +# perf wrappers (parallel to the flame-graph helper) +# ============================================================ +pg-perf-record() { + local pid + pid=$(pgrep -f "postgres.*-D.*$PG_DATA_DIR" | head -1) + if [ -z "$pid" ]; then + echo "No postgres running under $PG_DATA_DIR" >&2 + return 1 + fi + mkdir -p "$PG_BENCH_DIR" + local out="$PG_BENCH_DIR/perf-$(date +%Y%m%d_%H%M%S).data" + echo "Recording to $out (Ctrl-C to stop)" + perf record -F 997 --call-graph dwarf -p "$pid" -o "$out" "$@" + echo "Saved: $out" +} + +pg-perf-report() { + local data + data=$(ls -t "$PG_BENCH_DIR"/perf-*.data 2>/dev/null | head -1) + if [ -z "$data" ]; then + echo "No perf data in $PG_BENCH_DIR" >&2 + return 1 + fi + echo "Reading $data" + perf report -i "$data" "$@" +} + +pg-perf-annotate() { + local data + data=$(ls -t "$PG_BENCH_DIR"/perf-*.data 2>/dev/null | head -1) + if [ -z "$data" ]; then + echo "No perf data in $PG_BENCH_DIR" >&2 + return 1 + fi + perf annotate -i "$data" "$@" +} + +# ============================================================ +# Single regression test / group runner. +# Runs pg_regress directly against the existing build so you skip the +# full meson-driven suite wrapper. Usage: pg-test-one boolean [name ...] +# ============================================================ +pg-test-one() { + if [ $# -eq 0 ]; then + echo "usage: pg-test-one TESTNAME [TESTNAME ...]" + echo "example: pg-test-one boolean" + return 2 + fi + local bdir="${PG_BUILD_DIR_ONE:-$PG_BUILD_DIR}" + local tmpbin="$bdir/tmp_install$PG_INSTALL_DIR/bin" + if [ ! -x "$tmpbin/postgres" ]; then + echo "Populating tmp_install..." + meson test -C "$bdir" tmp_install install_test_files initdb_cache >/dev/null || return 1 + fi + local outdir + outdir=$(mktemp -d /tmp/pg-test-one-XXXXXX) + echo "Test output: $outdir" + "$bdir/src/test/regress/pg_regress" \ + --bindir="$tmpbin" \ + --inputdir="$PG_SOURCE_DIR/src/test/regress" \ + --expecteddir="$PG_SOURCE_DIR/src/test/regress" \ + --dlpath="$bdir/src/test/regress" \ + --outputdir="$outdir" \ + --temp-instance="$outdir/tmp" \ + --port=40099 \ + "$@" +} + +# Full flame graph / benchmark aliases +alias pg-flame='pg-flame-generate' +alias pg-flame-30='pg-flame-generate 30' +alias pg-flame-60='pg-flame-generate 60' +alias pg-flame-120='pg-flame-generate 120' + +pg-flame-custom() { + local duration=${1:-30} + local output_dir=${2:-$PG_FLAME_DIR} + echo "Generating flame graph for ${duration}s, output to: $output_dir" + pg-flame-generate "$duration" "$output_dir" +} + +alias pg-bench='pg-bench-run' +alias pg-bench-quick='pg-bench-run 5 1 100 1 30 select-only' +alias pg-bench-standard='pg-bench-run 10 2 1000 10 60 tpcb-like' +alias pg-bench-heavy='pg-bench-run 50 4 5000 100 300 tpcb-like' +alias pg-bench-readonly='pg-bench-run 20 4 2000 50 120 select-only' + +pg-bench-custom() { + local clients=${1:-10} + local threads=${2:-2} + local transactions=${3:-1000} + local scale=${4:-10} + local duration=${5:-60} + local test_type=${6:-tpcb-like} + + echo "Running custom benchmark:" + echo " Clients: $clients, Threads: $threads" + echo " Transactions: $transactions, Scale: $scale" + echo " Duration: ${duration}s, Type: $test_type" + + pg-bench-run "$clients" "$threads" "$transactions" "$scale" "$duration" "$test_type" +} + +pg-bench-flame() { + local duration=${1:-60} + local clients=${2:-10} + local scale=${3:-10} + + echo "Running benchmark with flame graph generation" + echo "Duration: ${duration}s, Clients: $clients, Scale: $scale" + + pg-bench-run "$clients" 2 1000 "$scale" "$duration" tpcb-like & + local bench_pid=$! + + sleep 5 + + local flame_duration=$((duration - 10)) + if [ $flame_duration -gt 10 ]; then + pg-flame-generate "$flame_duration" & + local flame_pid=$! + fi + + wait $bench_pid + if [ -n "${flame_pid:-}" ]; then + wait $flame_pid + fi + + echo "Benchmark and flame graph generation completed" +} + +# Live monitoring +alias pg-perf='perf top -p $(pgrep -f "postgres.*-D.*$PG_DATA_DIR" | head -1)' +alias pg-htop='htop -p $(pgrep -f "postgres.*-D.*$PG_DATA_DIR" | tr "\n" "," | sed "s/,$//")' + +pg-stats() { + local duration=${1:-30} + echo "Collecting system stats for ${duration}s..." + + iostat -x 1 "$duration" >"$PG_BENCH_DIR/iostat_$(date +%Y%m%d_%H%M%S).log" & + vmstat 1 "$duration" >"$PG_BENCH_DIR/vmstat_$(date +%Y%m%d_%H%M%S).log" & + + wait + echo "System stats saved to $PG_BENCH_DIR" +} + +# ============================================================ +# Code quality helpers +# ============================================================ +pg-format() { + local since=${1:-HEAD} + + if [ ! -f "$PG_SOURCE_DIR/src/tools/pgindent/pgindent" ]; then + echo "Error: pgindent not found at $PG_SOURCE_DIR/src/tools/pgindent/pgindent" + else + + modified_files=$(git diff --name-only "${since}" | grep -E "\.c$|\.h$") + + if [ -z "$modified_files" ]; then + echo "No modified .c or .h files found" + else + + echo "Formatting modified files with pgindent:" + for file in $modified_files; do + if [ -f "$file" ]; then + echo " Formatting: $file" + "$PG_SOURCE_DIR/src/tools/pgindent/pgindent" "$file" + else + echo " Warning: File not found: $file" + fi + done + + echo "Checking files for whitespace:" + git diff --check "${since}" + fi + fi +} + +pg-tidy() { + local since=${1:-HEAD} + local files + files=$(git diff --name-only "$since" | grep -E "\.(c|h)$") + if [ -z "$files" ]; then + echo "No modified .c or .h files." + return 0 + fi + for f in $files; do + [ -f "$f" ] || continue + echo "clang-tidy: $f" + clang-tidy -p "$PG_BUILD_DIR" "$f" 2>&1 | head -50 + done +} + +pg-spell() { + local since=${1:-HEAD} + local files=$(git diff --name-only "$since" | grep -E '\.(c|h|sgml|md)$') + if [ -z "$files" ]; then + echo "No .c/.h/.sgml/.md files changed since $since" + return 0 + fi + for f in $files; do + [ -f "$f" ] || continue + case "$f" in + *.c | *.h) + grep -nE '^\s*(/\*|\*|//)' "$f" | codespell --stdin-single-line - 2>/dev/null \ + && echo " $f: ok" || true + ;; + *.sgml | *.md) + codespell "$f" || true + ;; + esac + done +} + +# ============================================================ +# Core dump one-shots (one-time, requires root). kernel.core_pattern +# is a system-wide sysctl -- we don't touch it on every shell entry. +# ============================================================ +pg-cores-status() { + echo "ulimit -c: $(ulimit -c)" + echo "kernel.core_pattern: $(cat /proc/sys/kernel/core_pattern 2>/dev/null || echo unreadable)" + echo "cwd: $(pwd)" +} + +pg-enable-cores() { + ulimit -c unlimited + if ! [ -w /proc/sys/kernel/core_pattern ]; then + echo "Setting kernel.core_pattern (requires sudo)..." + echo "core.%p" | sudo tee /proc/sys/kernel/core_pattern >/dev/null || { + echo "Failed to write /proc/sys/kernel/core_pattern" >&2 + return 1 + } + else + echo "core.%p" >/proc/sys/kernel/core_pattern + fi + pg-cores-status +} + +pg-disable-cores() { + ulimit -c 0 + if ! [ -w /proc/sys/kernel/core_pattern ]; then + echo "Restoring kernel.core_pattern to 'core' (requires sudo)..." + echo "core" | sudo tee /proc/sys/kernel/core_pattern >/dev/null || { + echo "Failed to restore /proc/sys/kernel/core_pattern" >&2 + return 1 + } + else + echo "core" >/proc/sys/kernel/core_pattern + fi + pg-cores-status +} + +# ============================================================ +# Logs and results +# ============================================================ +alias pg-log='tail -f "$PG_DATA_DIR/log/postgresql-$(date +%Y-%m-%d).log" 2>/dev/null || echo "No log file found"' +alias pg-log-errors='grep -i error "$PG_DATA_DIR/log/"*.log 2>/dev/null || echo "No error logs found"' + +alias pg-build-log='cat "$PG_BUILD_DIR/meson-logs/meson-log.txt"' +alias pg-build-errors='grep -i error "$PG_BUILD_DIR/meson-logs/meson-log.txt" 2>/dev/null || echo "No build errors found"' + +alias pg-bench-results='ls -la "$PG_BENCH_DIR" && echo "Latest results:" && tail -20 "$PG_BENCH_DIR"/results_*.txt 2>/dev/null | tail -20' +alias pg-flame-results='ls -la "$PG_FLAME_DIR" && echo "Open flame graphs with: firefox $PG_FLAME_DIR/*.svg"' + +pg-clean-results() { + local days=${1:-7} + echo "Cleaning benchmark and flame graph results older than $days days..." + find "$PG_BENCH_DIR" -type f -mtime +$days -delete 2>/dev/null || true + find "$PG_FLAME_DIR" -type f -mtime +$days -delete 2>/dev/null || true + echo "Cleanup completed" +} + +# ============================================================ +# Info +# ============================================================ +alias pg-info=' + echo "=== PostgreSQL Development Environment ===" + echo "Source: $PG_SOURCE_DIR" + echo "Build (default): $PG_BUILD_DIR" + echo "Build (valgrind):$PG_BUILD_DIR_VALGRIND" + echo "Build (asan): $PG_BUILD_DIR_ASAN" + echo "Install: $PG_INSTALL_DIR" + echo "Data: $PG_DATA_DIR" + echo "Benchmarks: $PG_BENCH_DIR" + echo "Flame graphs: $PG_FLAME_DIR" + echo "Compiler: $CC" + echo "" + echo "Available commands:" + echo " Setup/build: pg-setup, pg-build, pg-install" + echo " Database: pg-init, pg-start, pg-stop, pg-psql" + echo " Tests: pg-test, pg-test-one NAME" + echo " Valgrind: pg-build-valgrind, pg-valgrind-regress, pg-valgrind-test" + echo " ASan/UBSan: pg-build-asan, pg-asan-regress" + echo " Debug: pg-debug, pg-attach" + echo " Record/replay: pg-rr-check, pg-rr-record, pg-rr-replay" + echo " Perf: pg-perf-record, pg-perf-report, pg-perf-annotate, pg-perf" + echo " Flame graphs: pg-flame, pg-flame-30, pg-flame-60, pg-flame-custom" + echo " Benchmarks: pg-bench-quick, pg-bench-standard, pg-bench-heavy" + echo " Combined: pg-bench-flame" + echo " Results: pg-bench-results, pg-flame-results" + echo " Logs: pg-log, pg-build-log" + echo " Clean: pg-clean, pg-full-clean, pg-clean-results" + echo " Code quality: pg-format, pg-tidy, pg-spell" + echo " Cores: pg-enable-cores, pg-disable-cores, pg-cores-status" + echo "=========================================="' + +echo "PostgreSQL aliases loaded. Run 'pg-info' for available commands." diff --git a/shell.nix b/shell.nix new file mode 100644 index 0000000000000..8c738afa789a7 --- /dev/null +++ b/shell.nix @@ -0,0 +1,745 @@ +{ + pkgs, + pkgs-unstable, + system, +}: let + # Create a patched glibc only for the dev shell. + # + # Glibc's features.h emits a `-Wcpp` diagnostic when _FORTIFY_SOURCE is + # defined without an optimization level. Meson's dependency probes + # (notably the libcurl thread-safety check) compile small snippets with + # `-O0 -Werror`, which turns that cpp warning into a hard error and + # breaks reconfigure under our default CFLAGS. The patch simply drops + # the warning. It is scoped to this dev shell only and never leaks + # into system glibc or release builds. + patchedGlibc = pkgs.glibc.overrideAttrs (oldAttrs: { + patches = (oldAttrs.patches or []) ++ [ + ./glibc-no-fortify-warning.patch + ]; + }); + + # Use LLVM for modern PostgreSQL development + llvmPkgs = pkgs-unstable.llvmPackages_21; + + # Configuration constants + config = { + pgSourceDir = "$PWD"; + pgBuildDir = "$PWD/build"; + pgBuildDirValgrind = "$PWD/build-valgrind"; + pgBuildDirAsan = "$PWD/build-asan"; + pgInstallDir = "$PWD/install"; + pgDataDir = "/tmp/test-db-$(basename $PWD)"; + pgBenchDir = "/tmp/pgbench-results-$(basename $PWD)"; + pgFlameDir = "/tmp/flame-graphs-$(basename $PWD)"; + }; + + # Single dependency function that can be used for all environments + getPostgreSQLDeps = muslLibs: + with pkgs; + [ + # Build system (always use host tools) + pkgs-unstable.meson + pkgs-unstable.ninja + pkg-config + autoconf + git + which + binutils + gnumake + mold # fast linker, big wins on large postgres links + + # Parser/lexer tools + bison + flex + + # Perl with required packages + (perl.withPackages (ps: with ps; [IPCRun])) + + # Documentation + docbook_xml_dtd_45 + docbook-xsl-nons + libxslt + libxml2 + fop + + # Development tools (always use host tools) + coreutils + shellcheck + ripgrep + valgrind + curl + uv + pylint + black + lcov + strace + ltrace + perf-tools + linuxPackages.perf + flamegraph + bpftrace # kernel-level tracing (probes, uprobes) + rr # record-and-replay deterministic debugger + htop + iotop + sysstat + ccache + cppcheck + compdb + + # Spell checking + aspell + aspellDicts.en + codespell + + # GCC/GDB + gcc + gdb + + # LLVM toolchain + llvmPkgs.llvm + llvmPkgs.llvm.dev + llvmPkgs.clang-tools + llvmPkgs.lldb + + # Language support + (python3.withPackages (ps: with ps; [requests browser-cookie3])) + tcl + ] + ++ ( + if muslLibs + then [ + # Musl target libraries for cross-compilation + pkgs.pkgsMusl.readline + pkgs.pkgsMusl.zlib + pkgs.pkgsMusl.openssl + pkgs.pkgsMusl.icu + pkgs.pkgsMusl.lz4 + pkgs.pkgsMusl.zstd + pkgs.pkgsMusl.libuuid + pkgs.pkgsMusl.libkrb5 + pkgs.pkgsMusl.linux-pam + pkgs.pkgsMusl.libxcrypt + ] + else [ + # Glibc target libraries + readline + zlib + openssl + icu + lz4 + zstd + libuuid + libkrb5 + linux-pam + libxcrypt + numactl + openldap + liburing + libselinux + patchedGlibc + patchedGlibc.dev + ] + ); + + # GDB configuration for PostgreSQL debugging + gdbConfig = pkgs.writeText "gdbinit-postgres" '' + # PostgreSQL-specific GDB configuration + + # Pretty-print PostgreSQL data structures + define print_node + if $arg0 + printf "Node type: %s\n", nodeTagNames[$arg0->type] + print *$arg0 + else + printf "NULL node\n" + end + end + document print_node + Print a PostgreSQL Node with type information + Usage: print_node + end + + define print_list + set $list = (List*)$arg0 + if $list + printf "List length: %d\n", $list->length + set $cell = $list->head + set $i = 0 + while $cell && $i < $list->length + printf " [%d]: ", $i + print_node $cell->data.ptr_value + set $cell = $cell->next + set $i = $i + 1 + end + else + printf "NULL list\n" + end + end + document print_list + Print a PostgreSQL List structure + Usage: print_list + end + + define print_query + set $query = (Query*)$arg0 + if $query + printf "Query type: %d, command type: %d\n", $query->querySource, $query->commandType + print *$query + else + printf "NULL query\n" + end + end + document print_query + Print a PostgreSQL Query structure + Usage: print_query + end + + define print_relcache + set $rel = (Relation)$arg0 + if $rel + printf "Relation: %s.%s (OID: %u)\n", $rel->rd_rel->relnamespace, $rel->rd_rel->relname.data, $rel->rd_id + printf " natts: %d, relkind: %c\n", $rel->rd_rel->relnatts, $rel->rd_rel->relkind + else + printf "NULL relation\n" + end + end + document print_relcache + Print relation cache entry information + Usage: print_relcache + end + + define print_tupdesc + set $desc = (TupleDesc)$arg0 + if $desc + printf "TupleDesc: %d attributes\n", $desc->natts + set $i = 0 + while $i < $desc->natts + set $attr = $desc->attrs[$i] + printf " [%d]: %s (type: %u, len: %d)\n", $i, $attr->attname.data, $attr->atttypid, $attr->attlen + set $i = $i + 1 + end + else + printf "NULL tuple descriptor\n" + end + end + document print_tupdesc + Print tuple descriptor information + Usage: print_tupdesc + end + + define print_slot + set $slot = (TupleTableSlot*)$arg0 + if $slot + printf "TupleTableSlot: %s\n", $slot->tts_ops->name + printf " empty: %d, shouldFree: %d\n", $slot->tts_empty, $slot->tts_shouldFree + if $slot->tts_tupleDescriptor + print_tupdesc $slot->tts_tupleDescriptor + end + else + printf "NULL slot\n" + end + end + document print_slot + Print tuple table slot information + Usage: print_slot + end + + # Memory context debugging + define print_mcxt + set $context = (MemoryContext)$arg0 + if $context + printf "MemoryContext: %s\n", $context->name + printf " type: %s, parent: %p\n", $context->methods->name, $context->parent + printf " total: %zu, free: %zu\n", $context->mem_allocated, $context->freep - $context->freeptr + else + printf "NULL memory context\n" + end + end + document print_mcxt + Print memory context information + Usage: print_mcxt + end + + # Process debugging + define print_proc + set $proc = (PGPROC*)$arg0 + if $proc + printf "PGPROC: pid=%d, database=%u\n", $proc->pid, $proc->databaseId + printf " waiting: %d, waitStatus: %d\n", $proc->waiting, $proc->waitStatus + else + printf "NULL process\n" + end + end + document print_proc + Print process information + Usage: print_proc + end + + # Set useful defaults + set print pretty on + set print object on + set print static-members off + set print vtbl on + set print demangle on + set demangle-style gnu-v3 + set print sevenbit-strings off + set history save on + set history size 1000 + set history filename ~/.gdb_history_postgres + + # Common breakpoints for PostgreSQL debugging + define pg_break_common + break elog + break errfinish + break ExceptionalCondition + break ProcessInterrupts + end + document pg_break_common + Set common PostgreSQL debugging breakpoints + end + + printf "PostgreSQL GDB configuration loaded.\n" + printf "Available commands: print_node, print_list, print_query, print_relcache,\n" + printf " print_tupdesc, print_slot, print_mcxt, print_proc, pg_break_common\n" + ''; + + # Flame graph generation script + flameGraphScript = pkgs.writeScriptBin "pg-flame-generate" '' + #!${pkgs.bash}/bin/bash + set -euo pipefail + + DURATION=''${1:-30} + OUTPUT_DIR=''${2:-${config.pgFlameDir}} + TIMESTAMP=$(date +%Y%m%d_%H%M%S) + + mkdir -p "$OUTPUT_DIR" + + echo "Generating flame graph for PostgreSQL (duration: ''${DURATION}s)" + + # Find PostgreSQL processes + PG_PIDS=$(pgrep -f "postgres.*-D.*${config.pgDataDir}" || true) + + if [ -z "$PG_PIDS" ]; then + echo "Error: No PostgreSQL processes found" + exit 1 + fi + + echo "Found PostgreSQL processes: $PG_PIDS" + + # Record perf data + PERF_DATA="$OUTPUT_DIR/perf_$TIMESTAMP.data" + echo "Recording perf data to $PERF_DATA" + + ${pkgs.linuxPackages.perf}/bin/perf record \ + -F 997 \ + -g \ + --call-graph dwarf \ + -p "$(echo $PG_PIDS | tr ' ' ',')" \ + -o "$PERF_DATA" \ + sleep "$DURATION" + + # Generate flame graph + FLAME_SVG="$OUTPUT_DIR/postgres_flame_$TIMESTAMP.svg" + echo "Generating flame graph: $FLAME_SVG" + + ${pkgs.linuxPackages.perf}/bin/perf script -i "$PERF_DATA" | \ + ${pkgs.flamegraph}/bin/stackcollapse-perf.pl | \ + ${pkgs.flamegraph}/bin/flamegraph.pl \ + --title "PostgreSQL Flame Graph ($TIMESTAMP)" \ + --width 1200 \ + --height 800 \ + > "$FLAME_SVG" + + echo "Flame graph generated: $FLAME_SVG" + echo "Perf data saved: $PERF_DATA" + + # Generate summary report + REPORT="$OUTPUT_DIR/report_$TIMESTAMP.txt" + echo "Generating performance report: $REPORT" + + { + echo "PostgreSQL Performance Analysis Report" + echo "Generated: $(date)" + echo "Duration: ''${DURATION}s" + echo "Processes: $PG_PIDS" + echo "" + echo "=== Top Functions ===" + ${pkgs.linuxPackages.perf}/bin/perf report -i "$PERF_DATA" --stdio --sort comm,dso,symbol | head -50 + echo "" + echo "=== Call Graph ===" + ${pkgs.linuxPackages.perf}/bin/perf report -i "$PERF_DATA" --stdio -g --sort comm,dso,symbol | head -100 + } > "$REPORT" + + echo "Report generated: $REPORT" + echo "" + echo "Files created:" + echo " Flame graph: $FLAME_SVG" + echo " Perf data: $PERF_DATA" + echo " Report: $REPORT" + ''; + + # pgbench wrapper script + pgbenchScript = pkgs.writeScriptBin "pg-bench-run" '' + #!${pkgs.bash}/bin/bash + set -euo pipefail + + # Default parameters + CLIENTS=''${1:-10} + THREADS=''${2:-2} + TRANSACTIONS=''${3:-1000} + SCALE=''${4:-10} + DURATION=''${5:-60} + TEST_TYPE=''${6:-tpcb-like} + + OUTPUT_DIR="${config.pgBenchDir}" + TIMESTAMP=$(date +%Y%m%d_%H%M%S) + + mkdir -p "$OUTPUT_DIR" + + echo "=== PostgreSQL Benchmark Configuration ===" + echo "Clients: $CLIENTS" + echo "Threads: $THREADS" + echo "Transactions: $TRANSACTIONS" + echo "Scale factor: $SCALE" + echo "Duration: ''${DURATION}s" + echo "Test type: $TEST_TYPE" + echo "Output directory: $OUTPUT_DIR" + echo "============================================" + + # Check if PostgreSQL is running + if ! pgrep -f "postgres.*-D.*${config.pgDataDir}" >/dev/null; then + echo "Error: PostgreSQL is not running. Start it with 'pg-start'" + exit 1 + fi + + PGBENCH="${config.pgInstallDir}/bin/pgbench" + PSQL="${config.pgInstallDir}/bin/psql" + CREATEDB="${config.pgInstallDir}/bin/createdb" + DROPDB="${config.pgInstallDir}/bin/dropdb" + + DB_NAME="pgbench_test_$TIMESTAMP" + RESULTS_FILE="$OUTPUT_DIR/results_$TIMESTAMP.txt" + LOG_FILE="$OUTPUT_DIR/pgbench_$TIMESTAMP.log" + + echo "Creating test database: $DB_NAME" + "$CREATEDB" -h "${config.pgDataDir}" "$DB_NAME" || { + echo "Failed to create database" + exit 1 + } + + # Initialize pgbench tables + echo "Initializing pgbench tables (scale factor: $SCALE)" + "$PGBENCH" -h "${config.pgDataDir}" -i -s "$SCALE" "$DB_NAME" || { + echo "Failed to initialize pgbench tables" + "$DROPDB" -h "${config.pgDataDir}" "$DB_NAME" 2>/dev/null || true + exit 1 + } + + # Run benchmark based on test type + echo "Running benchmark..." + + case "$TEST_TYPE" in + "tpcb-like"|"default") + BENCH_ARGS="" + ;; + "select-only") + BENCH_ARGS="-S" + ;; + "simple-update") + BENCH_ARGS="-N" + ;; + "read-write") + BENCH_ARGS="-b select-only@70 -b tpcb-like@30" + ;; + *) + echo "Unknown test type: $TEST_TYPE" + echo "Available types: tpcb-like, select-only, simple-update, read-write" + "$DROPDB" -h "${config.pgDataDir}" "$DB_NAME" 2>/dev/null || true + exit 1 + ;; + esac + + { + echo "PostgreSQL Benchmark Results" + echo "Generated: $(date)" + echo "Test type: $TEST_TYPE" + echo "Clients: $CLIENTS, Threads: $THREADS" + echo "Transactions: $TRANSACTIONS, Duration: ''${DURATION}s" + echo "Scale factor: $SCALE" + echo "Database: $DB_NAME" + echo "" + echo "=== System Information ===" + echo "CPU: $(nproc) cores" + echo "Memory: $(free -h | grep '^Mem:' | awk '{print $2}')" + echo "Compiler: $CC" + echo "PostgreSQL version: $("$PSQL" --no-psqlrc -h "${config.pgDataDir}" -d "$DB_NAME" -t -c "SELECT version();" | head -1)" + echo "" + echo "=== Benchmark Results ===" + } > "$RESULTS_FILE" + + # Run the actual benchmark + "$PGBENCH" \ + -h "${config.pgDataDir}" \ + -c "$CLIENTS" \ + -j "$THREADS" \ + -T "$DURATION" \ + -P 5 \ + --log \ + --log-prefix="$OUTPUT_DIR/pgbench_$TIMESTAMP" \ + $BENCH_ARGS \ + "$DB_NAME" 2>&1 | tee -a "$RESULTS_FILE" + + # Collect additional statistics + { + echo "" + echo "=== Database Statistics ===" + "$PSQL" --no-psqlrc -h "${config.pgDataDir}" -d "$DB_NAME" -c " + SELECT + schemaname, + relname, + n_tup_ins as inserts, + n_tup_upd as updates, + n_tup_del as deletes, + n_live_tup as live_tuples, + n_dead_tup as dead_tuples + FROM pg_stat_user_tables; + " + + echo "" + echo "=== Index Statistics ===" + "$PSQL" --no-psqlrc -h "${config.pgDataDir}" -d "$DB_NAME" -c " + SELECT + schemaname, + relname, + indexrelname, + idx_scan, + idx_tup_read, + idx_tup_fetch + FROM pg_stat_user_indexes; + " + } >> "$RESULTS_FILE" + + # Clean up + echo "Cleaning up test database: $DB_NAME" + "$DROPDB" -h "${config.pgDataDir}" "$DB_NAME" 2>/dev/null || true + + echo "" + echo "Benchmark completed!" + echo "Results saved to: $RESULTS_FILE" + echo "Transaction logs: $OUTPUT_DIR/pgbench_$TIMESTAMP*" + + # Show summary + echo "" + echo "=== Quick Summary ===" + grep -E "(tps|latency)" "$RESULTS_FILE" | tail -5 + ''; + + # Shared shellHook fragments. Each devShell prepends its own compiler/CFLAGS + # block, then appends the common tail via ${commonHookTail variant}. + commonHookHead = icon: '' + # History configuration + export HISTFILE=.history + export HISTSIZE=1000000 + export HISTFILESIZE=1000000 + + # Clean environment + unset LD_LIBRARY_PATH LD_PRELOAD LIBRARY_PATH C_INCLUDE_PATH CPLUS_INCLUDE_PATH + + # Essential tools in PATH + export PATH="${pkgs.which}/bin:${pkgs.coreutils}/bin:$PATH" + export PS1="$(echo -e '\u${icon}') {\[$(tput sgr0)\]\[\033[38;5;228m\]\w\[$(tput sgr0)\]\[\033[38;5;15m\]} ($(git rev-parse --abbrev-ref HEAD)) \\$ \[$(tput sgr0)\]" + + # Ccache configuration + export PATH=${pkgs.ccache}/bin:$PATH + export CCACHE_COMPILERCHECK=content + # Loosen a few rules so ccache hits across rebuilds with touched headers. + export CCACHE_SLOPPINESS=pch_defines,time_macros,include_file_mtime,include_file_ctime + export CCACHE_DIR=$HOME/.ccache/pg/$(basename $PWD) + mkdir -p "$CCACHE_DIR" + + # Development tools in PATH + export PATH=${pkgs.clang-tools}/bin:$PATH + export PATH=${pkgs.cppcheck}/bin:$PATH + ''; + + # Tail shared by every devShell: PG env vars, GDB, tool PATH, per-process + # setup and alias load. Kernel core_pattern is NOT touched here -- + # run 'pg-enable-cores' explicitly if you need per-PID cores in CWD. + commonHookTail = label: '' + # PostgreSQL environment + export PG_SOURCE_DIR="${config.pgSourceDir}" + export PG_BUILD_DIR="${config.pgBuildDir}" + export PG_BUILD_DIR_VALGRIND="${config.pgBuildDirValgrind}" + export PG_BUILD_DIR_ASAN="${config.pgBuildDirAsan}" + export PG_INSTALL_DIR="${config.pgInstallDir}" + export PG_DATA_DIR="${config.pgDataDir}" + export PG_BENCH_DIR="${config.pgBenchDir}" + export PG_FLAME_DIR="${config.pgFlameDir}" + export PERL_CORE_DIR=$(find ${pkgs.perl} -maxdepth 5 -path "*/CORE" -type d) + + # GDB configuration + export GDBINIT="${gdbConfig}" + + # Performance tools in PATH + export PATH="${flameGraphScript}/bin:${pgbenchScript}/bin:$PATH" + + # Create output directories + mkdir -p "$PG_BENCH_DIR" "$PG_FLAME_DIR" + + # Per-process core dump size limit. Kernel core_pattern is NOT + # touched here -- run 'pg-enable-cores' explicitly when you need + # per-PID cores in CWD. + ulimit -c unlimited + + # Local git excludes + git config core.excludesFile .local-gitignore 2>/dev/null || true + + # Load PostgreSQL development aliases + if [ -f ./pg-aliases.sh ]; then + source ./pg-aliases.sh + else + echo "Warning: pg-aliases.sh not found in current directory" + fi + + echo "" + echo "PostgreSQL Development Environment Ready (${label})" + echo "Run 'pg-info' for available commands" + ''; + + # Development shell (GCC + glibc) + devShell = pkgs.mkShell { + name = "postgresql-dev"; + buildInputs = + (getPostgreSQLDeps false) + ++ [ + flameGraphScript + pgbenchScript + ]; + + shellHook = + (commonHookHead "f121") + + '' + # LLVM configuration + export LLVM_CONFIG="${llvmPkgs.llvm}/bin/llvm-config" + export PATH="${llvmPkgs.llvm}/bin:$PATH" + export PKG_CONFIG_PATH="${llvmPkgs.llvm.dev}/lib/pkgconfig:$PKG_CONFIG_PATH" + export LLVM_DIR="${llvmPkgs.llvm.dev}/lib/cmake/llvm" + export LLVM_ROOT="${llvmPkgs.llvm}" + + # PostgreSQL Development CFLAGS + export CFLAGS="" + export CXXFLAGS="" + + # Python UV + UV_PYTHON_DOWNLOADS=never + + # GCC configuration (default compiler) + export CC="${pkgs.gcc}/bin/gcc" + export CXX="${pkgs.gcc}/bin/g++" + + echo "Environment configured:" + echo " Compiler: $CC" + echo " libc: glibc" + echo " LLVM: $(llvm-config --version 2>/dev/null || echo 'not available')" + '' + + (commonHookTail "GCC + glibc"); + }; + + # Clang + glibc variant + clangDevShell = pkgs.mkShell { + name = "postgresql-clang-glibc"; + buildInputs = + (getPostgreSQLDeps false) + ++ [ + llvmPkgs.clang + llvmPkgs.lld + llvmPkgs.compiler-rt + flameGraphScript + pgbenchScript + ]; + + shellHook = + (commonHookHead "f121") + + '' + # LLVM configuration + export LLVM_CONFIG="${llvmPkgs.llvm}/bin/llvm-config" + export PATH="${llvmPkgs.llvm}/bin:$PATH" + export PKG_CONFIG_PATH="${llvmPkgs.llvm.dev}/lib/pkgconfig:$PKG_CONFIG_PATH" + export LLVM_DIR="${llvmPkgs.llvm.dev}/lib/cmake/llvm" + export LLVM_ROOT="${llvmPkgs.llvm}" + + # Clang + glibc configuration + export CC="${llvmPkgs.clang}/bin/clang" + export CXX="${llvmPkgs.clang}/bin/clang++" + + echo "Environment configured:" + echo " Compiler: $CC" + echo " libc: glibc" + echo " LLVM: $(llvm-config --version 2>/dev/null || echo 'not available')" + '' + + (commonHookTail "Clang + glibc"); + }; + + # GCC + musl variant (cross-compilation) + muslDevShell = pkgs.mkShell { + name = "postgresql-gcc-musl"; + buildInputs = + (getPostgreSQLDeps true) + ++ [ + pkgs.gcc + flameGraphScript + pgbenchScript + ]; + + shellHook = + (commonHookHead "f121") + + '' + # Cross-compilation to musl with GCC + export CC="${pkgs.gcc}/bin/gcc" + export CXX="${pkgs.gcc}/bin/g++" + + export PKG_CONFIG_PATH="${pkgs.pkgsMusl.openssl.dev}/lib/pkgconfig:${pkgs.pkgsMusl.zlib.dev}/lib/pkgconfig:${pkgs.pkgsMusl.icu.dev}/lib/pkgconfig" + export CFLAGS="-ggdb -Og -fno-omit-frame-pointer -D_FORTIFY_SOURCE=1 -I${pkgs.pkgsMusl.stdenv.cc.libc}/include" + export CXXFLAGS="-ggdb -Og -fno-omit-frame-pointer -D_FORTIFY_SOURCE=1 -I${pkgs.pkgsMusl.stdenv.cc.libc}/include" + export LDFLAGS="-L${pkgs.pkgsMusl.stdenv.cc.libc}/lib -static-libgcc" + + echo "Environment configured:" + echo " Compiler: $CC" + echo " libc: musl (cross-compilation)" + '' + + (commonHookTail "GCC + musl"); + }; + + # Clang + musl variant (cross-compilation) + clangMuslDevShell = pkgs.mkShell { + name = "postgresql-clang-musl"; + buildInputs = + (getPostgreSQLDeps true) + ++ [ + llvmPkgs.clang + llvmPkgs.lld + flameGraphScript + pgbenchScript + ]; + + shellHook = + (commonHookHead "f121") + + '' + # Cross-compilation to musl with clang + export CC="${llvmPkgs.clang}/bin/clang" + export CXX="${llvmPkgs.clang}/bin/clang++" + + export PKG_CONFIG_PATH="${pkgs.pkgsMusl.openssl.dev}/lib/pkgconfig:${pkgs.pkgsMusl.zlib.dev}/lib/pkgconfig:${pkgs.pkgsMusl.icu.dev}/lib/pkgconfig" + export CFLAGS="--target=x86_64-linux-musl -ggdb -Og -fno-omit-frame-pointer -D_FORTIFY_SOURCE=1 -I${pkgs.pkgsMusl.stdenv.cc.libc}/include" + export CXXFLAGS="--target=x86_64-linux-musl -ggdb -Og -fno-omit-frame-pointer -D_FORTIFY_SOURCE=1 -I${pkgs.pkgsMusl.stdenv.cc.libc}/include" + export LDFLAGS="--target=x86_64-linux-musl -L${pkgs.pkgsMusl.stdenv.cc.libc}/lib -fuse-ld=lld" + + echo "Environment configured:" + echo " Compiler: $CC" + echo " libc: musl (cross-compilation)" + '' + + (commonHookTail "Clang + musl"); + }; +in { + inherit devShell clangDevShell muslDevShell clangMuslDevShell gdbConfig flameGraphScript pgbenchScript; +} diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 21d00f792d24e..bfd7e84e56bef 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -1243,7 +1243,7 @@ spawn_process(const char *cmdline) char *cmdline2; cmdline2 = psprintf("exec %s", cmdline); - execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); + execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); /* Not using the normal bail() here as we want _exit */ bail_noatexit("could not exec \"%s\": %m", shellprog); } diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 004b8fcab0027..747f054351486 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -1,4 +1,4 @@ -#!/usr/bin/perl +#!/usr/bin/env perl # Copyright (c) 2021-2026, PostgreSQL Global Development Group From 64d4776ff84dba00d237273b0dab345e4ad9f62f Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 6 Jul 2026 17:25:00 -0400 Subject: [PATCH 6/8] Batch the clock sweep to reduce nextVictimBuffer atomic contention StrategyGetBuffer() advances the shared clock hand, nextVictimBuffer, with a pg_atomic_fetch_add_u32(..., 1) on every tick. On a multi-socket system the cache line holding that counter has to travel over the interconnect on each operation, pushing a sweep tick from ~20ns (same-socket, line warm in L1/L2) into the ~100-200ns range. Under eviction pressure with hundreds of backends in StrategyGetBuffer() concurrently, that single cache line becomes the dominant cost of the sweep, visible as elevated bus-cycles and cache-misses in a perf profile. Have each backend claim a run of consecutive buffer IDs from the shared hand with a single fetch-add and then iterate through them privately. The sweep still advances through the pool in order, each buffer is still visited exactly once per complete pass, and the meaning of the clock state is unchanged; only the temporal ordering of visits within a pass changes, which the algorithm does not depend on. The contended atomic now fires roughly once per batch rather than once per buffer. The batch is one cache line's worth of clock-hand values -- PG_CACHE_LINE_SIZE / sizeof(uint32) -- capped at NBuffers so a claim can never wrap the pool more than once. Batching only helps when the counter's cache line actually bounces between sockets, so it is enabled only on multi-node NUMA hardware (pg_numa_get_max_node() >= 1); on a single socket, or where libnuma is unavailable, the batch size stays 1 and the code path is byte-identical to the stock clock sweep. Wraparound handling is adjusted: with batching, several backends can each see a fetch-add return a value past NBuffers within the same pass. Any such backend takes buffer_strategy_lock, re-reads the counter, and if it is still out of range wraps it with a single CAS and increments completePasses. StrategySyncStart() continues to see a consistent (nextVictimBuffer, completePasses) pair. This is the batched-clock-sweep idea from Jim Mlodgenski's pgsql-hackers thread, adapted to derive the batch size from the platform cache-line size. Co-authored-by: Jim Mlodgenski --- src/backend/storage/buffer/freelist.c | 152 ++++++++++++++++++-------- 1 file changed, 109 insertions(+), 43 deletions(-) diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index fdb5bad7910a2..46104b3341d61 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -22,6 +22,7 @@ #include "storage/proc.h" #include "storage/shmem.h" #include "storage/subsystems.h" +#include "port/pg_numa.h" #define INT_ACCESS_ONCE(var) ((int)(*((volatile int *)&(var)))) @@ -100,68 +101,110 @@ static BufferDesc *GetBufferFromRing(BufferAccessStrategy strategy, static void AddBufferToRing(BufferAccessStrategy strategy, BufferDesc *buf); +/* + * Per-backend state for the batched clock sweep. Each backend claims a run + * of consecutive clock-hand values with a single atomic fetch-add and then + * iterates through them privately, so the contended nextVictimBuffer cache + * line is touched roughly 1/batch as often. MyBatchPos is the next hand + * value to hand out; MyBatchEnd is one past the end of the claimed run. Both + * are absolute (monotonically increasing) hand values; the buffer id is the + * value modulo NBuffers. + */ +static uint32 MyBatchPos = 0; +static uint32 MyBatchEnd = 0; + +/* + * Number of clock-hand values a backend claims per atomic fetch-add, + * computed once at startup (see StrategyCtlShmemInit). When batching is + * enabled it is one cache line's worth of hand advance, so concurrent + * backends sweep non-overlapping, cache-line-sized runs of the pool; the + * global sweep order is preserved (each buffer is still visited exactly once + * per pass). Batching is enabled only on multi-node NUMA hardware; otherwise + * this stays 1 and the sweep is byte-identical to the stock clock. + */ +static uint32 ClockSweepBatchSize = 1; + /* * ClockSweepTick - Helper routine for StrategyGetBuffer() * - * Move the clock hand one buffer ahead of its current position and return the - * id of the buffer now under the hand. + * Return the next buffer to consider for eviction. Backends claim batches of + * consecutive buffer IDs from the shared clock hand, then iterate through + * them locally without further atomic operations. This preserves the global + * sweep order while reducing contention on the shared counter. */ static inline uint32 ClockSweepTick(void) { uint32 victim; - /* - * Atomically move hand ahead one buffer - if there's several processes - * doing this, this can lead to buffers being returned slightly out of - * apparent order. - */ - victim = - pg_atomic_fetch_add_u32(&StrategyControl->nextVictimBuffer, 1); - - if (victim >= NBuffers) + if (MyBatchPos >= MyBatchEnd) { - uint32 originalVictim = victim; - - /* always wrap what we look up in BufferDescriptors */ - victim = victim % NBuffers; - /* - * If we're the one that just caused a wraparound, force - * completePasses to be incremented while holding the spinlock. We - * need the spinlock so StrategySyncStart() can return a consistent - * value consisting of nextVictimBuffer and completePasses. + * Claim a fresh batch from the shared clock hand. This is the only + * atomic operation per batch, reducing contention by the batch size. */ - if (victim == 0) - { - uint32 expected; - uint32 wrapped; - bool success = false; + uint32 start; + uint32 batch_size = ClockSweepBatchSize; - expected = originalVictim + 1; + start = pg_atomic_fetch_add_u32(&StrategyControl->nextVictimBuffer, + batch_size); - while (!success) - { - /* - * Acquire the spinlock while increasing completePasses. That - * allows other readers to read nextVictimBuffer and - * completePasses in a consistent manner which is required for - * StrategySyncStart(). In theory delaying the increment - * could lead to an overflow of nextVictimBuffers, but that's - * highly unlikely and wouldn't be particularly harmful. - */ - SpinLockAcquire(&StrategyControl->buffer_strategy_lock); + if (start >= (uint32) NBuffers) + { + start = start % NBuffers; - wrapped = expected % NBuffers; + /* + * The counter has grown past NBuffers; try to wrap it back. We + * must hold the spinlock so StrategySyncStart() can read + * nextVictimBuffer and completePasses consistently. + * + * With batching, multiple backends may each land a fetch-add + * that returns a value past NBuffers in the same pass. After + * acquiring the spinlock we re-read the counter: if another + * backend already wrapped it below NBuffers we are done. + */ + SpinLockAcquire(&StrategyControl->buffer_strategy_lock); + { + uint32 current; + uint32 wrapped; - success = pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, - &expected, wrapped); - if (success) - StrategyControl->completePasses++; - SpinLockRelease(&StrategyControl->buffer_strategy_lock); + current = pg_atomic_read_u32(&StrategyControl->nextVictimBuffer); + if (current >= (uint32) NBuffers) + { + wrapped = current % NBuffers; + if (pg_atomic_compare_exchange_u32(&StrategyControl->nextVictimBuffer, + ¤t, wrapped)) + StrategyControl->completePasses++; + } } + SpinLockRelease(&StrategyControl->buffer_strategy_lock); + } + else if (start + batch_size > (uint32) NBuffers) + { + /* + * The fetch-add returned a value below NBuffers, but this batch + * spans the wrap point (start .. NBuffers-1 then 0 .. k are + * iterated locally via "% NBuffers"). That crossing is a + * complete pass too, but the wrap-and-increment path above only + * fires when the fetch-add return is itself >= NBuffers, so + * without this it would go uncounted and completePasses would + * drift low (it feeds bgwriter pacing / StrategySyncStart, not + * correctness). The batch is capped at NBuffers, so a batch + * spans the wrap at most once; count it under the spinlock for a + * consistent (nextVictimBuffer, completePasses) pair. + */ + SpinLockAcquire(&StrategyControl->buffer_strategy_lock); + StrategyControl->completePasses++; + SpinLockRelease(&StrategyControl->buffer_strategy_lock); } + + MyBatchPos = start; + MyBatchEnd = start + batch_size; } + + victim = MyBatchPos % NBuffers; + MyBatchPos++; + return victim; } @@ -408,6 +451,29 @@ StrategyCtlShmemInit(void *arg) /* No pending notification */ StrategyControl->bgwprocno = -1; + + /* + * Decide whether to batch the clock sweep. + * + * Batching claims a run of consecutive buffer IDs per atomic fetch-add so + * concurrent backends touch the shared nextVictimBuffer cache line ~1/batch + * as often -- a win only when that line actually bounces across sockets, + * i.e. on multi-node NUMA hardware. On a single socket the atomic is + * already node-local and batching would only make backends skip ahead for + * no benefit, so we fall back to batch size 1 there (byte-identical to the + * stock one-buffer-at-a-time clock sweep). + * + * Batch (> 1) only when libnuma reports more than one node + * (pg_numa_get_max_node() >= 1); pg_numa_init() returns -1 when NUMA is + * unavailable. (Benchmarking showed the win holds with or without huge + * pages, so huge-page availability is intentionally not part of the gate.) + */ + if (pg_numa_init() != -1 && + pg_numa_get_max_node() >= 1) + ClockSweepBatchSize = Min(PG_CACHE_LINE_SIZE / (uint32) sizeof(uint32), + (uint32) NBuffers); + else + ClockSweepBatchSize = 1; } From 08b5782e2da29c2c80fdf66eb17b46e378eb10c5 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 6 Jul 2026 19:22:45 -0400 Subject: [PATCH 7/8] Replace the usage_count clock sweep with a cooling-stage evictor Replace the 0..5 usage_count buffer-replacement policy with a cooling-stage clock (the LeanStore / 2Q-A1 model): a buffer is simply HOT (recently used) or COOL (an eviction candidate), with "pinned" being the existing refcount. There is no per-buffer access counter. - A demand-loaded page is admitted COOL (probationary), not HOT. A second access via PinBuffer promotes it COOL -> HOT (the rescue). So a page touched once -- a sequential scan -- fills and drains the COOL stage and is evicted from it without ever displacing the HOT working set. Scan resistance is intrinsic to the replacement algorithm, which is what lets a later commit remove the BufferAccessStrategy ring buffers entirely. - The foreground sweep in StrategyGetBuffer() reclaims an already-COOL, unpinned buffer, pinning it with a CAS so a racing PinBuffer always wins. Promotion (COOL -> HOT) and demotion (HOT -> COOL) are single-bit transitions; only the eviction claim is a CAS. - The background writer maintains the supply of COOL victims. As its LRU scan runs ahead of the clock hand it demotes HOT buffers to COOL, so the foreground finds a victim in a single pass rather than having to cool buffers itself. The demotion is demand-driven -- bounded by the predicted allocation for the next cycle -- so it stages just enough COOL buffers without cooling the whole pool, and it is done under the buffer header lock the scan already holds. A single second-chance reference bit (set by PinBuffer, cleared on the bgwriter's first pass over a HOT buffer) spares a recently-accessed buffer one cooling pass, keeping the genuinely hot set out of the COOL stage under scan pressure. BgBufferSync also tracks the reusable-buffer density it directly observes on a shorter smoothing window, so a burst of probationary/scan COOL pages is followed promptly rather than averaged away. Under a bulk-dirtying workload the per-cycle clean-write cap is raised from bgwriter_lru_maxpages to predicted demand so the bgwriter keeps supplying clean victims, rather than the foreground sweep having to flush dirty victims inline; normal workloads, where demand is below the cap, are unaffected. The 4-bit usage_count field is reinterpreted in place: bit 0 is the HOT/COOL state (BUF_COOLSTATE_ONE), bit 1 the reference bit (BUF_REFBIT). The 64-bit buffer-state layout -- refcount, flag and lock offsets and their StaticAsserts -- is unchanged; only the meaning of the field and the instructions that touch it change. A StaticAssert requires the field to be at least 2 bits wide so a future width change cannot push the reference bit into the flag bits. BM_MAX_USAGE_COUNT becomes BUF_COOLSTATE_HOT (1), so the pin fast path saturates at HOT. Local (temp-table) buffers get the same two-state treatment; being single-backend they need no background cooler. Because the reference bit shares the field, the full 4-bit value can be 0..3; readers that mean "the cooling state" must use BUF_STATE_GET_COOLSTATE(), which masks to bit 0 and returns only 0 (COOL) or 1 (HOT), never the raw field. contrib/pg_buffercache is updated accordingly: its usagecount column and the pg_buffercache_summary average report the cooling state (0 = COOL, 1 = HOT), and pg_buffercache_usage_counts() buckets on it. Using the raw 4-bit getter there would index its BM_MAX_USAGE_COUNT+1 = 2-element arrays with values up to 3 and overrun the stack; masking to the cooling bit keeps the index in range. The reference bit is deliberately not exposed as usagecount. Depends on the batched clock sweep from the previous commit. --- contrib/pg_buffercache/pg_buffercache_pages.c | 6 +- src/backend/storage/buffer/bufmgr.c | 130 ++++++++++++++---- src/backend/storage/buffer/freelist.c | 114 +++++++++------ src/backend/storage/buffer/localbuf.c | 13 +- src/include/storage/buf_internals.h | 63 +++++++-- 5 files changed, 239 insertions(+), 87 deletions(-) diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c b/contrib/pg_buffercache/pg_buffercache_pages.c index 510455998aa74..b94e6f6fda80b 100644 --- a/contrib/pg_buffercache/pg_buffercache_pages.c +++ b/contrib/pg_buffercache/pg_buffercache_pages.c @@ -161,7 +161,7 @@ pg_buffercache_pages(PG_FUNCTION_ARGS) reldatabase = bufHdr->tag.dbOid; forknum = BufTagGetForkNum(&bufHdr->tag); blocknum = bufHdr->tag.blockNum; - usagecount = BUF_STATE_GET_USAGECOUNT(buf_state); + usagecount = BUF_STATE_GET_COOLSTATE(buf_state); pinning_backends = BUF_STATE_GET_REFCOUNT(buf_state); if (buf_state & BM_DIRTY) @@ -605,7 +605,7 @@ pg_buffercache_summary(PG_FUNCTION_ARGS) if (buf_state & BM_VALID) { buffers_used++; - usagecount_total += BUF_STATE_GET_USAGECOUNT(buf_state); + usagecount_total += BUF_STATE_GET_COOLSTATE(buf_state); if (buf_state & BM_DIRTY) buffers_dirty++; @@ -655,7 +655,7 @@ pg_buffercache_usage_counts(PG_FUNCTION_ARGS) CHECK_FOR_INTERRUPTS(); - usage_count = BUF_STATE_GET_USAGECOUNT(buf_state); + usage_count = BUF_STATE_GET_COOLSTATE(buf_state); usage_counts[usage_count]++; if (buf_state & BM_DIRTY) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 3908529872a31..86892667cadf9 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -83,6 +83,7 @@ /* Bits in SyncOneBuffer's return value */ #define BUF_WRITTEN 0x01 #define BUF_REUSABLE 0x02 +#define BUF_COOLED 0x04 #define RELS_BSEARCH_THRESHOLD 20 @@ -634,7 +635,7 @@ static void PinBuffer_Locked(BufferDesc *buf); static void UnpinBuffer(BufferDesc *buf); static void UnpinBufferNoOwner(BufferDesc *buf); static void BufferSync(int flags); -static int SyncOneBuffer(int buf_id, bool skip_recently_used, +static int SyncOneBuffer(int buf_id, bool skip_recently_used, bool cool_if_hot, WritebackContext *wb_context); static void WaitIO(BufferDesc *buf); static void AbortBufferIO(Buffer buffer); @@ -2333,7 +2334,10 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, * checkpoints, except for their "init" forks, which need to be treated * just like permanent relations. */ - set_bits |= BM_TAG_VALID | BUF_USAGECOUNT_ONE; + set_bits |= BM_TAG_VALID; + /* Admit the newly loaded page COOL (probation); a second access via + * PinBuffer promotes it to HOT. This is what makes a one-touch scan + * self-evicting -- see the cooling-state notes in buf_internals.h. */ if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM) set_bits |= BM_PERMANENT; @@ -3002,7 +3006,9 @@ ExtendBufferedRelShared(BufferManagerRelation bmr, victim_buf_hdr->tag = tag; - set_bits |= BM_TAG_VALID | BUF_USAGECOUNT_ONE; + set_bits |= BM_TAG_VALID; + /* Admit COOL (probation); see the comment at the other admission + * site and the cooling-state notes in buf_internals.h. */ if (bmr.relpersistence == RELPERSISTENCE_PERMANENT || fork == INIT_FORKNUM) set_bits |= BM_PERMANENT; @@ -3332,21 +3338,17 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy, /* increase refcount */ buf_state += BUF_REFCOUNT_ONE; - if (strategy == NULL) - { - /* Default case: increase usagecount unless already max. */ - if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT) - buf_state += BUF_USAGECOUNT_ONE; - } - else - { - /* - * Ring buffers shouldn't evict others from pool. Thus we - * don't make usagecount more than 1. - */ - if (BUF_STATE_GET_USAGECOUNT(buf_state) == 0) - buf_state += BUF_USAGECOUNT_ONE; - } + /* + * Accessing a resident buffer promotes it to HOT (the 2Q rescue): + * a page loaded COOL on probation becomes part of the hot working + * set on its second touch. BM_MAX_USAGE_COUNT is + * BUF_COOLSTATE_HOT (1), so this saturates at HOT and never + * overflows the field. We also set the second-chance ref bit so + * the bgwriter's next cooling pass spares this recently-used buffer. + */ + if (BUF_STATE_GET_COOLSTATE(buf_state) < BUF_COOLSTATE_HOT) + buf_state += BUF_COOLSTATE_ONE; + buf_state |= BUF_REFBIT; if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, buf_state)) @@ -3785,7 +3787,7 @@ BufferSync(int flags) */ if (pg_atomic_read_u64(&bufHdr->state) & BM_CHECKPOINT_NEEDED) { - if (SyncOneBuffer(buf_id, false, &wb_context) & BUF_WRITTEN) + if (SyncOneBuffer(buf_id, false, false, &wb_context) & BUF_WRITTEN) { TRACE_POSTGRESQL_BUFFER_SYNC_WRITTEN(buf_id); PendingCheckpointerStats.buffers_written++; @@ -3876,6 +3878,19 @@ BgBufferSync(WritebackContext *wb_context) float smoothing_samples = 16; float scan_whole_pool_milliseconds = 120000.0; + /* + * The cleaner scan directly observes the reusable (COOL, unpinned) buffer + * density over the region it walks, which -- with the cooling-stage + * evictor -- is exactly the sweep's victim predicate. That observation is + * ground truth for the buffers about to be reused, whereas the strategy + * scan's positional proxy (strategy_delta/recent_alloc) blurs a pool whose + * COOL population is spatially clustered (a scan burst leaves whole regions + * COOL, hot OLTP regions not). So we let the cleaner's own sample adapt on + * a shorter window than the strategy proxy, tracking a burst of + * probationary/scan COOL pages within a cycle or two instead of lagging it. + */ + float cleaner_smoothing_samples = 4; + /* Used to compute how far we scan ahead */ long strategy_delta; int bufs_to_lap; @@ -3889,6 +3904,7 @@ BgBufferSync(WritebackContext *wb_context) int num_to_scan; int num_written; int reusable_buffers; + int write_limit; /* Variables for final smoothed_density update */ long new_strategy_delta; @@ -4063,8 +4079,22 @@ BgBufferSync(WritebackContext *wb_context) * Now write out dirty reusable buffers, working forward from the * next_to_clean point, until we have lapped the strategy scan, or cleaned * enough buffers to match our estimate of the next cycle's allocation - * requirements, or hit the bgwriter_lru_maxpages limit. - */ + * requirements, or hit the write limit. + * + * The per-cycle write cap is normally bgwriter_lru_maxpages. But under a + * bulk-dirtying workload (COPY, bulk UPDATE, VACUUM) the pool fills with + * dirty COOL buffers faster than that fixed cap can clean, so the + * foreground clock sweep is forced to flush dirty victims inline -- the + * very cost the cooling-stage evictor is meant to keep off the critical + * path. When predicted demand (upcoming_alloc_est) exceeds the fixed cap, + * raise the limit to meet demand so the bgwriter stays ahead and supplies + * clean victims. This stays bounded (by demand and by lapping the + * strategy point), so it cannot run away, and normal workloads are + * unaffected because there upcoming_alloc_est <= bgwriter_lru_maxpages. + */ + write_limit = bgwriter_lru_maxpages; + if (upcoming_alloc_est > write_limit) + write_limit = upcoming_alloc_est; num_to_scan = bufs_to_lap; num_written = 0; @@ -4073,7 +4103,13 @@ BgBufferSync(WritebackContext *wb_context) /* Execute the LRU scan */ while (num_to_scan > 0 && reusable_buffers < upcoming_alloc_est) { - int sync_state = SyncOneBuffer(next_to_clean, true, + /* + * cool_if_hot = false: with the cool-in-place foreground sweep, the + * clock hand demotes HOT buffers on every tick, so the bgwriter does + * writeback only and leaves all cooling to the sweep (no background + * pre-cooling, no double-cooling). + */ + int sync_state = SyncOneBuffer(next_to_clean, true, false, wb_context); if (++next_to_clean >= NBuffers) @@ -4086,7 +4122,7 @@ BgBufferSync(WritebackContext *wb_context) if (sync_state & BUF_WRITTEN) { reusable_buffers++; - if (++num_written >= bgwriter_lru_maxpages) + if (++num_written >= write_limit) { PendingBgWriterStats.maxwritten_clean++; break; @@ -4121,7 +4157,7 @@ BgBufferSync(WritebackContext *wb_context) { scans_per_alloc = (float) new_strategy_delta / (float) new_recent_alloc; smoothed_density += (scans_per_alloc - smoothed_density) / - smoothing_samples; + cleaner_smoothing_samples; #ifdef BGW_DEBUG elog(DEBUG2, "bgwriter: cleaner density alloc=%u scan=%ld density=%.2f new smoothed=%.2f", @@ -4140,16 +4176,26 @@ BgBufferSync(WritebackContext *wb_context) * If skip_recently_used is true, we don't write currently-pinned buffers, nor * buffers marked recently used, as these are not replacement candidates. * + * If cool_if_hot is true (the bgwriter's LRU scan), an unpinned HOT buffer is + * demoted HOT -> COOL as we pass it, pre-staging eviction candidates so the + * foreground clock sweep finds a COOL victim in a single pass instead of + * having to cool buffers itself (force_cool). The demotion is done under the + * buffer header lock we already hold, so it needs no CAS and cannot race a + * concurrent demotion. A concurrent PinBuffer promotes it back to HOT, which + * is the intended 2Q behavior (a re-accessed buffer is rescued). + * * Returns a bitmask containing the following flag bits: * BUF_WRITTEN: we wrote the buffer. * BUF_REUSABLE: buffer is available for replacement, ie, it has - * pin count 0 and usage count 0. + * pin count 0 and is COOL (an eviction candidate). + * BUF_COOLED: we demoted this buffer HOT -> COOL this call. * * (BUF_WRITTEN could be set in error if FlushBuffer finds the buffer clean * after locking it, but we don't care all that much.) */ static int -SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context) +SyncOneBuffer(int buf_id, bool skip_recently_used, bool cool_if_hot, + WritebackContext *wb_context) { BufferDesc *bufHdr = GetBufferDescriptor(buf_id); int result = 0; @@ -4171,8 +4217,38 @@ SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context) */ buf_state = LockBufHdr(bufHdr); + /* + * Pre-cool with a second chance: if asked, act on an unpinned HOT buffer. + * If its ref bit is set (accessed since our last pass), clear the ref bit + * and leave it HOT -- a recently-used buffer earns one reprieve, keeping + * the hot working set out of the COOL stage under scan pressure. Only a + * HOT buffer whose ref bit is already clear is demoted HOT -> COOL, + * pre-staging it as an eviction candidate for the foreground sweep. We + * hold the header lock, so each transition is a plain masked store applied + * atomically by UnlockBufHdrExt; we re-lock to continue the dirty-write + * inspection below. + */ + if (cool_if_hot && + BUF_STATE_GET_REFCOUNT(buf_state) == 0 && + BUF_STATE_GET_COOLSTATE(buf_state) != BUF_COOLSTATE_COOL) + { + if (BUF_STATE_GET_REFBIT(buf_state)) + { + /* second chance: consume the ref bit, stay HOT */ + UnlockBufHdrExt(bufHdr, buf_state, 0, BUF_REFBIT, 0); + buf_state = LockBufHdr(bufHdr); + } + else + { + /* not re-accessed since last pass: demote to COOL */ + UnlockBufHdrExt(bufHdr, buf_state, 0, BUF_USAGECOUNT_MASK, 0); + buf_state = LockBufHdr(bufHdr); + result |= BUF_COOLED; + } + } + if (BUF_STATE_GET_REFCOUNT(buf_state) == 0 && - BUF_STATE_GET_USAGECOUNT(buf_state) == 0) + BUF_STATE_GET_COOLSTATE(buf_state) == BUF_COOLSTATE_COOL) { result |= BUF_REUSABLE; } diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index 46104b3341d61..808f13360e62a 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -49,6 +49,16 @@ typedef struct uint32 completePasses; /* Complete cycles of the clock-sweep */ pg_atomic_uint32 numBufferAllocs; /* Buffers allocated since last reset */ + /* + * Number of clock-hand values a backend claims per atomic fetch-add. + * Computed once at startup (see StrategyCtlShmemInit). Kept in shared + * memory rather than a backend-local static so that EXEC_BACKEND children + * (which do not inherit the postmaster's statics) see the same value; a + * backend-local copy would silently reset to 1 there, disabling batching + * on Windows. + */ + uint32 batchSize; + /* * Bgworker process to be notified upon activity or -1 if none. See * StrategyNotifyBgWriter. @@ -113,17 +123,6 @@ static void AddBufferToRing(BufferAccessStrategy strategy, static uint32 MyBatchPos = 0; static uint32 MyBatchEnd = 0; -/* - * Number of clock-hand values a backend claims per atomic fetch-add, - * computed once at startup (see StrategyCtlShmemInit). When batching is - * enabled it is one cache line's worth of hand advance, so concurrent - * backends sweep non-overlapping, cache-line-sized runs of the pool; the - * global sweep order is preserved (each buffer is still visited exactly once - * per pass). Batching is enabled only on multi-node NUMA hardware; otherwise - * this stays 1 and the sweep is byte-identical to the stock clock. - */ -static uint32 ClockSweepBatchSize = 1; - /* * ClockSweepTick - Helper routine for StrategyGetBuffer() * @@ -144,7 +143,7 @@ ClockSweepTick(void) * atomic operation per batch, reducing contention by the batch size. */ uint32 start; - uint32 batch_size = ClockSweepBatchSize; + uint32 batch_size = StrategyControl->batchSize; start = pg_atomic_fetch_add_u32(&StrategyControl->nextVictimBuffer, batch_size); @@ -279,12 +278,30 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r */ pg_atomic_fetch_add_u32(&StrategyControl->numBufferAllocs, 1); - /* Use the "clock sweep" algorithm to find a free buffer */ + /* + * Use the cooling-stage clock sweep to find a victim. + * + * A buffer is HOT (recently used) or COOL (an eviction candidate). This + * is a COOL-IN-PLACE sweep: every tick past a HOT buffer demotes it one + * step toward COOL (respecting a single second-chance reference bit), and + * a COOL, unpinned buffer is claimed as the victim. Because every tick + * makes cooling progress, the sweep is self-staging: it never needs a + * background pre-cooler to keep COOL victims available, and it cannot get + * stuck in a "no COOL victim anywhere" state that forces a foreground + * escalation pass. Newly loaded pages are admitted COOL (see + * BufferAlloc), so scan resistance falls out of the algorithm. + * + * trycounter bounds the search: only a pinned buffer is "no progress". + * Demoting or reclaiming is progress and resets it. A full NBuffers pass + * that finds every buffer pinned fails, matching the stock "no unpinned + * buffers available" contract. + */ trycounter = NBuffers; for (;;) { uint64 old_buf_state; uint64 local_buf_state; + bool no_progress = false; buf = GetBufferDescriptor(ClockSweepTick()); @@ -297,25 +314,10 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r { local_buf_state = old_buf_state; - /* - * If the buffer is pinned or has a nonzero usage_count, we cannot - * use it; decrement the usage_count (unless pinned) and keep - * scanning. - */ - + /* If the buffer is pinned we cannot use it; keep scanning. */ if (BUF_STATE_GET_REFCOUNT(local_buf_state) != 0) { - if (--trycounter == 0) - { - /* - * We've scanned all the buffers without making any state - * changes, so all the buffers are pinned (or were when we - * looked at them). We could hope that someone will free - * one eventually, but it's probably better to fail than - * to risk getting stuck in an infinite loop. - */ - elog(ERROR, "no unpinned buffers available"); - } + no_progress = true; break; } @@ -326,9 +328,23 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r continue; } - if (BUF_STATE_GET_USAGECOUNT(local_buf_state) != 0) + if (BUF_STATE_GET_COOLSTATE(local_buf_state) != BUF_COOLSTATE_COOL) { - local_buf_state -= BUF_USAGECOUNT_ONE; + /* + * HOT buffer: cool it in place this tick. Apply a single + * second-chance reference bit: a HOT buffer whose ref bit is + * set (touched since it was last passed) has the ref bit + * cleared and stays HOT; only a HOT buffer whose ref bit is + * already clear is demoted HOT -> COOL. Either transition is + * progress toward a victim, so reset trycounter. We do NOT + * claim the buffer this tick -- a demoted buffer becomes a + * candidate for a later tick, giving it one more full sweep of + * grace before eviction. + */ + if (BUF_STATE_GET_REFBIT(local_buf_state)) + local_buf_state &= ~BUF_REFBIT; /* second chance: clear ref, stay HOT */ + else + local_buf_state &= ~BUF_USAGECOUNT_MASK; /* HOT -> COOL, clear ref bit */ if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, local_buf_state)) @@ -339,7 +355,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r } else { - /* pin the buffer if the CAS succeeds */ + /* COOL and unpinned: claim it. Pin if the CAS succeeds. */ local_buf_state += BUF_REFCOUNT_ONE; if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, @@ -356,6 +372,15 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r } } } + + /* + * Only a pinned buffer is no progress. A full NBuffers pass that + * makes no progress means every buffer is pinned, so fail rather than + * spin forever. (A failed CAS above is neither progress nor a full + * miss: we simply retry the same buffer.) + */ + if (no_progress && --trycounter == 0) + elog(ERROR, "no unpinned buffers available"); } } @@ -470,10 +495,10 @@ StrategyCtlShmemInit(void *arg) */ if (pg_numa_init() != -1 && pg_numa_get_max_node() >= 1) - ClockSweepBatchSize = Min(PG_CACHE_LINE_SIZE / (uint32) sizeof(uint32), - (uint32) NBuffers); + StrategyControl->batchSize = Min(PG_CACHE_LINE_SIZE / (uint32) sizeof(uint32), + (uint32) NBuffers); else - ClockSweepBatchSize = 1; + StrategyControl->batchSize = 1; } @@ -721,14 +746,17 @@ GetBufferFromRing(BufferAccessStrategy strategy, uint64 *buf_state) /* * If the buffer is pinned we cannot use it under any circumstances. * - * If usage_count is 0 or 1 then the buffer is fair game (we expect 1, - * since our own previous usage of the ring element would have left it - * there, but it might've been decremented by clock-sweep since then). - * A higher usage_count indicates someone else has touched the buffer, - * so we shouldn't re-use it. + * If it is unpinned but has been promoted to HOT, another backend + * touched it since we last cycled past this ring slot, so it has + * joined the working set and we must not recycle it -- tell the caller + * to get a fresh victim from the sweep instead. This is the 1-bit + * cooling-state analog of the stock ring's "usage_count > 1 means + * someone else touched it" test: a slot the ring keeps reusing and + * nobody else pins stays COOL, and an out-of-ring PinBuffer() is + * exactly what promotes it to HOT. */ - if (BUF_STATE_GET_REFCOUNT(local_buf_state) != 0 - || BUF_STATE_GET_USAGECOUNT(local_buf_state) > 1) + if (BUF_STATE_GET_REFCOUNT(local_buf_state) != 0 || + BUF_STATE_GET_COOLSTATE(local_buf_state) != BUF_COOLSTATE_COOL) break; /* See equivalent code in PinBuffer() */ diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 4870c8e13d010..21c08091c490b 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -167,7 +167,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, buf_state = pg_atomic_read_u64(&bufHdr->state); buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK); - buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE; + buf_state |= BM_TAG_VALID; /* admit COOL (probation) */ pg_atomic_unlocked_write_u64(&bufHdr->state, buf_state); *foundPtr = false; @@ -248,9 +248,10 @@ GetLocalVictimBuffer(void) { uint64 buf_state = pg_atomic_read_u64(&bufHdr->state); - if (BUF_STATE_GET_USAGECOUNT(buf_state) > 0) + if (BUF_STATE_GET_COOLSTATE(buf_state) != BUF_COOLSTATE_COOL) { - buf_state -= BUF_USAGECOUNT_ONE; + /* HOT: give it a second chance, cool it and keep scanning. */ + buf_state -= BUF_COOLSTATE_ONE; pg_atomic_unlocked_write_u64(&bufHdr->state, buf_state); trycounter = NLocBuffer; } @@ -454,7 +455,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr, victim_buf_hdr->tag = tag; - buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE; + buf_state |= BM_TAG_VALID; /* admit COOL (probation) */ pg_atomic_unlocked_write_u64(&victim_buf_hdr->state, buf_state); @@ -839,9 +840,9 @@ PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount) NLocalPinnedBuffers++; buf_state += BUF_REFCOUNT_ONE; if (adjust_usagecount && - BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT) + BUF_STATE_GET_COOLSTATE(buf_state) < BUF_COOLSTATE_HOT) { - buf_state += BUF_USAGECOUNT_ONE; + buf_state += BUF_COOLSTATE_ONE; } pg_atomic_unlocked_write_u64(&buf_hdr->state, buf_state); diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index e4ff5619b79ca..131df126cda00 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -67,6 +67,50 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS + BUF_L #define BUF_USAGECOUNT_ONE \ (UINT64CONST(1) << BUF_REFCOUNT_BITS) +/* + * Cooling state (LeanStore / 2Q-A1 cooling-stage clock sweep). + * + * The field historically used for the 0..5 usage_count now holds a single + * cooling-state bit: HOT (recently accessed, not an eviction candidate) or + * COOL (an eviction candidate). We reuse BUF_USAGECOUNT_ONE as the unit so + * the buffer-state bit geography -- refcount, flag, and lock offsets, and the + * 64-bit StaticAsserts -- is unchanged; only the meaning of the field and the + * instructions that touch it change. + * + * A demand-loaded page is admitted COOL (probation); a second access promotes + * it to HOT (the rescue). The sweep prefers COOL victims and demotes HOT to + * COOL only when a full pass finds no COOL victim. So a one-touch scan fills + * and drains the COOL stage without displacing the HOT working set -- scan + * resistance intrinsic to the replacement algorithm. + */ +#define BUF_COOLSTATE_COOL 0 +#define BUF_COOLSTATE_HOT 1 +#define BUF_COOLSTATE_ONE BUF_USAGECOUNT_ONE + +/* + * Second-chance reference bit, bit 1 of the (former usagecount) field, one + * position above the cooling-state bit. PinBuffer sets it on every access. + * The bgwriter's pre-cooling gives a HOT buffer a second chance: the first + * time it passes a HOT buffer whose ref bit is set, it clears the ref bit and + * leaves the buffer HOT; only a HOT buffer whose ref bit is already clear (not + * re-accessed since the previous bgwriter pass) is demoted to COOL. This + * keeps genuinely-hot pages out of the COOL stage (protecting the working set + * from being cooled under scan pressure) while leaving the foreground sweep a + * single-pass search over the pre-staged COOL buffers. A separate bit (not a + * count) so it stays a plain masked store under the header lock. + */ +#define BUF_REFBIT (UINT64CONST(2) << BUF_REFCOUNT_BITS) +#define BUF_STATE_GET_REFBIT(state) (((state) & BUF_REFBIT) != 0) + +/* + * The cooling state occupies bit 0 and the reference bit occupies bit 1 of + * the (former usagecount) field, so the field must be at least 2 bits wide. + * Assert it here so a future change to BUF_USAGECOUNT_BITS cannot silently + * push BUF_REFBIT up into the flag bits. + */ +StaticAssertDecl(BUF_USAGECOUNT_BITS >= 2, + "cooling state + reference bit need at least 2 bits in the usagecount field"); + /* flags related definitions */ #define BUF_FLAG_SHIFT \ (BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS) @@ -92,6 +136,11 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS + BUF_L #define BUF_STATE_GET_USAGECOUNT(state) \ ((uint32)(((state) & BUF_USAGECOUNT_MASK) >> BUF_USAGECOUNT_SHIFT)) +/* Cooling state (HOT/COOL) from buffer state -- bit 0 of the field only, so + * the second-chance ref bit (bit 1) does not perturb the HOT/COOL test. */ +#define BUF_STATE_GET_COOLSTATE(state) \ + ((uint32) (((state) >> BUF_USAGECOUNT_SHIFT) & 1)) + /* * Flags for buffer descriptors * @@ -134,17 +183,15 @@ StaticAssertDecl(MAX_BACKENDS_BITS <= (BUF_LOCK_BITS - 2), /* - * The maximum allowed value of usage_count represents a tradeoff between - * accuracy and speed of the clock-sweep buffer management algorithm. A - * large value (comparable to NBuffers) would approximate LRU semantics. - * But it can take as many as BM_MAX_USAGE_COUNT+1 complete cycles of the - * clock-sweep hand to find a free buffer, so in practice we don't want the - * value to be very large. + * The cooling state is a single bit (HOT/COOL); the maximum value stored in + * the field is therefore BUF_COOLSTATE_HOT. Retained under the historical + * name BM_MAX_USAGE_COUNT so the pin fast path ("promote unless already at + * max") reads naturally. */ -#define BM_MAX_USAGE_COUNT 5 +#define BM_MAX_USAGE_COUNT BUF_COOLSTATE_HOT StaticAssertDecl(BM_MAX_USAGE_COUNT < (UINT64CONST(1) << BUF_USAGECOUNT_BITS), - "BM_MAX_USAGE_COUNT doesn't fit in BUF_USAGECOUNT_BITS bits"); + "cooling state doesn't fit in BUF_USAGECOUNT_BITS bits"); /* * Buffer tag identifies which disk block the buffer contains. From 0fbdb6fc4857df3c95423410de39a89d1f1cf662 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 27 Jul 2026 14:07:10 -0400 Subject: [PATCH 8/8] Remove the BAS_BULKREAD ring; scan resistance is intrinsic to the evictor The cooling-stage evictor admits demand-loaded pages in the probationary COOL state and only promotes to HOT on a second touch, so a large single-pass scan fills and drains the COOL stage without displacing the hot working set. That makes the dedicated 256KB BAS_BULKREAD ring redundant: GetAccessStrategy(BAS_BULKREAD) now returns the default (NULL) strategy, so bulk reads use the normal allocation path. The write-side strategies BAS_BULKWRITE and BAS_VACUUM keep their rings. Those bound the writeback a bulk writer generates (the writer flushes and WAL-flushes its own dirty pages inline instead of dirtying all of shared_buffers and offloading the work), which the evictor does not provide and which reviewers correctly flagged as worth preserving. StrategyRejectBuffer() existed only to keep a BAS_BULKREAD scan from flushing a dirty victim inline; with no read ring it is unreachable and is removed, along with its call site in GetVictimBuffer(). The BufferAccessStrategyType enum and the IOCONTEXT_BULKREAD statistics context are retained so existing callers and pg_stat_io stay stable. --- src/backend/storage/buffer/bufmgr.c | 29 +++----- src/backend/storage/buffer/freelist.c | 98 ++++----------------------- src/include/storage/buf_internals.h | 2 - 3 files changed, 20 insertions(+), 109 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 86892667cadf9..4a1a29e5a0c9c 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2615,24 +2615,13 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) } /* - * If using a nondefault strategy, and this victim came from the - * strategy ring, let the strategy decide whether to reject it when - * reusing it would require a WAL flush. This only applies to - * permanent buffers; unlogged buffers can have fake LSNs, so - * XLogNeedsFlush() is not meaningful for them. - * - * We need to hold the content lock in at least share-exclusive mode - * to safely inspect the page LSN, so this couldn't have been done - * inside StrategyGetBuffer(). + * We hold the content lock in share-exclusive mode (acquired above) so + * it is safe to flush the dirty victim. (Bulk reads no longer use a + * ring or reject dirty victims -- scan resistance is intrinsic to the + * cooling evictor -- so there is no strategy-reject step here anymore; + * the write-side strategies always write their own dirty victims + * inline, which is their backpressure.) */ - if (strategy && from_ring && - buf_state & BM_PERMANENT && - XLogNeedsFlush(BufferGetLSN(buf_hdr)) && - StrategyRejectBuffer(strategy, buf_hdr, from_ring)) - { - UnlockReleaseBuffer(buf); - goto again; - } /* OK, do the I/O */ FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context); @@ -4689,10 +4678,8 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, * being used, this is counted as an IOCONTEXT_NORMAL IOOP_WRITE. * * If a shared buffer which was added to the ring later because the - * current strategy buffer is pinned or in use or because all strategy - * buffers were dirty and rejected (for BAS_BULKREAD operations only) - * requires flushing, this is counted as an IOCONTEXT_NORMAL IOOP_WRITE - * (from_ring will be false). + * current strategy buffer is pinned or in use requires flushing, this is + * counted as an IOCONTEXT_NORMAL IOOP_WRITE (from_ring will be false). * * When a strategy is not in use, the write can only be a "regular" write * of a dirty shared buffer (IOCONTEXT_NORMAL IOOP_WRITE). diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index 808f13360e62a..b730acaf38981 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -520,9 +520,6 @@ GetAccessStrategy(BufferAccessStrategyType btype) /* * Select ring size to use. See buffer/README for rationales. - * - * Note: if you change the ring size for BAS_BULKREAD, see also - * SYNC_SCAN_REPORT_INTERVAL in access/heap/syncscan.c. */ switch (btype) { @@ -531,50 +528,20 @@ GetAccessStrategy(BufferAccessStrategyType btype) return NULL; case BAS_BULKREAD: - { - int ring_max_kb; - - /* - * The ring always needs to be large enough to allow some - * separation in time between providing a buffer to the user - * of the strategy and that buffer being reused. Otherwise the - * user's pin will prevent reuse of the buffer, even without - * concurrent activity. - * - * We also need to ensure the ring always is large enough for - * SYNC_SCAN_REPORT_INTERVAL, as noted above. - * - * Thus we start out a minimal size and increase the size - * further if appropriate. - */ - ring_size_kb = 256; - - /* - * There's no point in a larger ring if we won't be allowed to - * pin sufficiently many buffers. But we never limit to less - * than the minimal size above. - */ - ring_max_kb = GetPinLimit() * (BLCKSZ / 1024); - ring_max_kb = Max(ring_size_kb, ring_max_kb); - /* - * We would like the ring to additionally have space for the - * configured degree of IO concurrency. While being read in, - * buffers can obviously not yet be reused. - * - * Each IO can be up to io_combine_limit blocks large, and we - * want to start up to effective_io_concurrency IOs. - * - * Note that effective_io_concurrency may be 0, which disables - * AIO. - */ - ring_size_kb += (BLCKSZ / 1024) * - io_combine_limit * effective_io_concurrency; + /* + * Bulk reads no longer use a ring. The cooling-stage evictor + * admits demand-loaded pages in the probationary COOL state, so a + * large single-pass scan fills and drains the COOL stage without + * displacing the hot working set: scan resistance is intrinsic to + * replacement and the read ring is redundant. (BAS_BULKWRITE and + * BAS_VACUUM keep their rings; those bound writer writeback, which + * the evictor does not do.) The type is retained so existing + * callers and the IOCONTEXT_BULKREAD statistics column keep + * working; it just maps to the default strategy now. + */ + return NULL; - if (ring_size_kb > ring_max_kb) - ring_size_kb = ring_max_kb; - break; - } case BAS_BULKWRITE: ring_size_kb = 16 * 1024; break; @@ -669,15 +636,6 @@ GetAccessStrategyPinLimit(BufferAccessStrategy strategy) switch (strategy->btype) { - case BAS_BULKREAD: - - /* - * Since BAS_BULKREAD uses StrategyRejectBuffer(), dirty buffers - * shouldn't be a problem and the caller is free to pin up to the - * entire ring at once. - */ - return strategy->nbuffers; - default: /* @@ -830,35 +788,3 @@ IOContextForStrategy(BufferAccessStrategy strategy) elog(ERROR, "unrecognized BufferAccessStrategyType: %d", strategy->btype); pg_unreachable(); } - -/* - * StrategyRejectBuffer -- consider rejecting a dirty buffer - * - * When a nondefault strategy is used, the buffer manager calls this function - * when it turns out that the buffer selected by StrategyGetBuffer needs to - * be written out and doing so would require flushing WAL too. This gives us - * a chance to choose a different victim. - * - * Returns true if buffer manager should ask for a new victim, and false - * if this buffer should be written and re-used. - */ -bool -StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, bool from_ring) -{ - /* We only do this in bulkread mode */ - if (strategy->btype != BAS_BULKREAD) - return false; - - /* Don't muck with behavior of normal buffer-replacement strategy */ - if (!from_ring || - strategy->buffers[strategy->current] != BufferDescriptorGetBuffer(buf)) - return false; - - /* - * Remove the dirty buffer from the ring; necessary to prevent infinite - * loop if all ring members are dirty. - */ - strategy->buffers[strategy->current] = InvalidBuffer; - - return true; -} diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 131df126cda00..c8d4ec3e62444 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -635,8 +635,6 @@ extern void TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint64 set_flag extern IOContext IOContextForStrategy(BufferAccessStrategy strategy); extern BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_ring); -extern bool StrategyRejectBuffer(BufferAccessStrategy strategy, - BufferDesc *buf, bool from_ring); extern int StrategySyncStart(uint32 *complete_passes, uint32 *num_buf_alloc); extern void StrategyNotifyBgWriter(int bgwprocno);