Skip to content

chore: bump xpg - #263

Merged
AndrewJackson2020 merged 1 commit into
masterfrom
bump_xpg
Nov 10, 2026
Merged

AndrewJackson2020 merged 1 commit into
masterfrom
bump_xpg

Conversation

@AndrewJackson2020

@AndrewJackson2020 AndrewJackson2020 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

This PR bumps xpg from 2.5.0 to 2.6.0. Along with this bump comes bumping of every postgres version from the initial release to the most current release. This bumping of the postgres minor versions caused a lot of drift in the required .out.patch files for the core tests.

@AndrewJackson2020
AndrewJackson2020 requested a review from a team as a code owner October 26, 2026 15:54
@AndrewJackson2020

AndrewJackson2020 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Interestingly it looks like tests are failing now. We bumped postgres minor versions a lot with this. Wondering if this is causing issues here.

Threw a breakpoint on and it looks like queries to views no longer trigger the ExecutorStart_hook for whatever reason.

Also on the platform it seems like this is basically only used for Postgrest roles. I wonder if this functionality is even needed at this point.

@steve-chavez do you have any insight here?

bash-5.3$ cat regression.diffs
diff -U3 /Users/aj/Repos/mono/external/repos/supabase/supautils/master/test/expected/permission_hints.out /Users/aj/Repos/mono/external/repos/supabase/supautils/master/results/permission_hints.out
--- /Users/aj/Repos/mono/external/repos/supabase/supautils/master/test/expected/permission_hints.out	2026-10-26 11:27:11.174378118 -0500
+++ /Users/aj/Repos/mono/external/repos/supabase/supautils/master/results/permission_hints.out	2026-10-26 11:27:14.736847677 -0500
@@ -127,7 +127,6 @@
 -- views rely on the owner's privileges, ensure hints still fire
 select * from hint_view;
 ERROR:  permission denied for view hint_view
-HINT:  Grant the required privileges to the current role with: GRANT SELECT ON public.hint_view TO hint_role;
 \echo

 reset role;
show supautils.hint_roles ;
       supautils.hint_roles
-----------------------------------
 anon, authenticated, service_role
(1 row)

@steve-chavez

Copy link
Copy Markdown
Member

Threw a breakpoint on and it looks like queries to views no longer trigger the ExecutorStart_hook for whatever reason.

Right, I think now we need to use the planner hook for the custom errors. But it's confusing to me why this happened on new minor versions, I'd assume this would only happen for pg 19.

Also on the platform it seems like this is basically only used for Postgrest roles. I wonder if this functionality is even needed at this point.

Yes, it's needed because of https://github.com/orgs/supabase/discussions/45329. It's still a big breaking change on the supabase platform.

I also tried to upstream this feature (ref) piece by piece but left it incomplete due to lack of time. That would certainly be better because overriding errors is quite hackish via extensions today.

@AndrewJackson2020

Copy link
Copy Markdown
Contributor Author

It's still a big breaking change on the supabase platform.

Idk if it's a breaking change if it's already broken lol. Good reason why we should be bumping dependencies regularly. I'm surprised this wasn't caught in the supabase/postgres repo CI. Complicating this is back compatibility. I would not like to have to build/test on every postgres minor version, most postgres extensions only support the latest minor release (afaik).

duckdb/pg_duckdb#1007

@steve-chavez

Copy link
Copy Markdown
Member

Idk if it's a breaking change if it's already broken lol.

😆 Right, but we really need to keep them for guiding the user during this transition of "no privileges by default". TBH since most users are using AI I'm not sure if it's that relevant anymore, so maybe we can drop the feature on a later major release.

@steve-chavez

Copy link
Copy Markdown
Member

(Also open to discussing if these enhanced hints belong in supautils or not, in hindsight it might have been a stretch and should rather live in another extension)

@AndrewJackson2020

Copy link
Copy Markdown
Contributor Author

Also open to discussing if these enhanced hints belong in supautils or not

I feel like there should probably be a discussion about what is supautils and what belongs in it in general. I originally thought that the goal was "provide certain non superusers access to stuff only superusers can do in a safe way". Stuff like this definitely does not belong in that category but im not sure where the boundary is exactly.

@steve-chavez

Copy link
Copy Markdown
Member

Stuff like this definitely does not belong in that category but im not sure where the boundary is exactly.

Opened #264 to discuss that.

@AndrewJackson2020

AndrewJackson2020 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

lldb analysis with breakpoint on function errcode.

when running SELECT * FROM hint_view;

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 4.1
   * frame #0: 0x00000001029cdb2c postgres`errcode
     frame #1: 0x000000010250a290 postgres`aclcheck_error + 1500
     frame #2: 0x00000001027494e8 postgres`subquery_planner + 868
     frame #3: 0x0000000102748b54 postgres`standard_planner + 344
     frame #4: 0x000000010284e034 postgres`pg_plan_query + 136
     frame #5: 0x000000010284e128 postgres`pg_plan_queries + 100
     frame #6: 0x0000000102852708 postgres`exec_simple_query + 1096
     frame #7: 0x0000000102850458 postgres`PostgresMain + 3368
     frame #8: 0x000000010284ac44 postgres`BackendMain + 140
     frame #9: 0x0000000102793e84 postgres`postmaster_child_launch + 288
     frame #10: 0x0000000102798528 postgres`ServerLoop + 6912
     frame #11: 0x00000001027960ec postgres`PostmasterMain + 4456
     frame #12: 0x000000010269a520 postgres`main + 1048
     frame #13: 0x00000001851404e4 dyld`start + 6992

when running SELECT * FROM hint_target;

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 5.1
   * frame #0: 0x00000001029cdb2c postgres`errcode
     frame #1: 0x000000010250a290 postgres`aclcheck_error + 1500
     frame #2: 0x0000000102638588 postgres`ExecCheckPermissions + 396
     frame #3: 0x00000001026376cc postgres`standard_ExecutorStart + 492
     frame #4: 0x00000001031a3c74 supautils.dylib`supautils_executor_start(queryDesc=0x0000000c3d19b900, eflags=0) at supautils.c:189:9 [opt]
     frame #5: 0x0000000102853488 postgres`PortalStart + 540
     frame #6: 0x000000010285276c postgres`exec_simple_query + 1196
     frame #7: 0x0000000102850458 postgres`PostgresMain + 3368
     frame #8: 0x000000010284ac44 postgres`BackendMain + 140
     frame #9: 0x0000000102793e84 postgres`postmaster_child_launch + 288
     frame #10: 0x0000000102798528 postgres`ServerLoop + 6912
     frame #11: 0x00000001027960ec postgres`PostmasterMain + 4456
     frame #12: 0x000000010269a520 postgres`main + 1048
     frame #13: 0x00000001851404e4 dyld`start + 6992

looking at git blame I think this change happened here postgres/postgres@2242495. It was the result of a CVE which is why this breaking change occurred during a minor release. The commit message states, in part:

    Checking view permissions at planner-startup in this way is a little
    ugly, since the same checks will be repeated at executor-startup.
    Longer-term, it might be better to move all the permissions checks
    from the executor to the planner so that permissions errors can be
    reported sooner, instead of creating a plan that won't ever be run.
    However, such a change seems too far-reaching to be back-patched.

so it sounds like eventually none of the hint functionality will work.

Also it seems like this issue is related to #182

@AndrewJackson2020

Copy link
Copy Markdown
Contributor Author

The code needed to implement the enhanced hints for views became way too out of scope of this PR. I opened #267 to deal with this.

@AndrewJackson2020
AndrewJackson2020 merged commit a8c13f6 into master Nov 10, 2026
21 checks passed
@AndrewJackson2020
AndrewJackson2020 deleted the bump_xpg branch November 10, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants