[AndrewJackson2020]

When attempting to bump postgres minor versions in supautils CI in #263 we discovered that the planner hook functionality was broken for more recent releases of postgres minor versions. We knew that this was broken for postgres major versions 18 and 19 but the patch that caused those to be broken was a CVE fix so they were also applied to postgres minor versions 17, 16, etc.

This PR attempts to fix this. Basically the issue is that for views ACL is checked during the planning stage and it never gets to the execution stage. As a result we need a new planner hook.

Another option maybe we should deprecate this functionality entirely as #264 suggests. See also #182.

[AndrewJackson2020]

Question for the reviewers: do we need to validate the performance before merging this? How risky is it adding an extra hook with this PR?

[steve-chavez]

@AndrewJackson2020 There are pgbench tests on https://github.com/supabase/supautils/actions/runs/41881805022?pr=267#summary-126755889292 to see the performance impact

[steve-chavez]

Results look a bit flaky, perhaps you can try them locally?

[AndrewJackson2020]

Results look a bit flaky, perhaps you can try them locally?

I think I fixed it. Might raise it as a separate PR.

[AndrewJackson2020]

I think I fixed it. Might raise it as a separate PR.

#271

[AndrewJackson2020]

Results look a bit flaky, perhaps you can try them locally?

I think we probably shouldn't be running benchmarks on GitHub runners. At a minimum we should be running on blacksmith. Not sure if running on my macOS laptop is the best either. Some issues that come up for me:

• Does the laptop sleeping have an impact on performance
• Since these are going to be run on linux in prod environments we probably want to run benchmarking on linux machines

I don't have a better solution to this right now though.

That said here are the results that I see attached when running locally. Probably the most concerning thing that I see is from supautils-loadtest-select which actually tests the hint behavior.

fix_view_hints branch: tps = 42831.875820 (without initial connection time)
master branch: tps = .222303 (without initial connection time)

Around a 3-4 percent drop in perf. Is this acceptable in exchange for fixing a broken feature?

supautils_load_test.md

[steve-chavez]

At a minimum we should be running on blacksmith

Agree, at least for the loadtests we should switch.

Around a 3-4 percent drop in perf. Is this acceptable in exchange for fixing a broken feature?

It's around the same impact I noticed before with the executor hook, should be fine since it only affects the roles in hint_roles.

[utkarash2991]

@AndrewJackson2020 any blocker for merging this one ? Asking as this is needed by : #260 .

[AndrewJackson2020]

@AndrewJackson2020 any blocker for merging this one ? Asking as this is needed by : #260 .

I don't think so, just need 10 minutes to do a final checkout of the code.

[AndrewJackson2020]

Only thing that I can see here that can be improved is there is a lot of duplicated code between supautils_planner and the executor planner. I think duplicated code in 2 places is probably fine for now though.