Thread: postgres_fdw - make cached connection functions tests meaningful
Hi, While working on [1], I got to know that there is a new GUC debug_invalidate_system_caches_always that has been introduced in v14. It can be used to switch off cache invalidation in CLOBBER_CACHE_ALWAYS builds which makes cache sensitive tests stable. Using this GUC, it is quite possible to make cached connection management function tests more meaningful by returning original values(true/false, all the output columns) instead of SELECT 1. Note that the commit f77717b29 stabilized the tests for those functions - postgres_fdw_disconnect, postgres_fdw_disconnect_all and postgres_fdw_get_connections by masking actual return value of the functions. Attaching a patch to use the new GUC to make the functions return actual output. Thoughts? [1] - https://www.postgresql.org/message-id/CALj2ACVGSQsq68y-LmyXKZzbNVgSgsiVKSzsrVXzVgnsZTN26Q%40mail.gmail.com With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Attachment
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> While working on [1], I got to know that there is a new GUC
> debug_invalidate_system_caches_always that has been introduced in v14.
> It can be used to switch off cache invalidation in
> CLOBBER_CACHE_ALWAYS builds which makes cache sensitive tests stable.
> Using this GUC, it is quite possible to make cached connection
> management function tests more meaningful by returning original
> values(true/false, all the output columns) instead of SELECT 1.
Note that this needs an update in the wake of d68a00391.
More generally, though, I am not sure that I believe the premise of
this patch. AFAICS it's assuming that forcing debug_discard_caches
off guarantees zero cache flushes, which it does not. (If it could,
we wouldn't need the whole thing; the point of that variable is to
deterministically force flushes which would otherwise be
nondeterministic, not nonexistent.) Even in a contrib test that
seemingly has nothing else running, background activity such as
autovacuum could result in surprises. So I fear that what you have
got here is a patch that will work 99% of the time; which is not
good enough for the buildfarm.
regards, tom lane
On Mon, May 10, 2021 at 6:03 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > Hi, > > While working on [1], I got to know that there is a new GUC > debug_invalidate_system_caches_always that has been introduced in v14. > It can be used to switch off cache invalidation in > CLOBBER_CACHE_ALWAYS builds which makes cache sensitive tests stable. > Using this GUC, it is quite possible to make cached connection > management function tests more meaningful by returning original > values(true/false, all the output columns) instead of SELECT 1. Note > that the commit f77717b29 stabilized the tests for those functions - > postgres_fdw_disconnect, postgres_fdw_disconnect_all and > postgres_fdw_get_connections by masking actual return value of the > functions. > > Attaching a patch to use the new GUC to make the functions return actual output. The patch does not apply on Head anymore, could you rebase and post a patch. I'm changing the status to "Waiting for Author". Regards, Vignesh
On Thu, Jul 15, 2021 at 3:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: > > While working on [1], I got to know that there is a new GUC > > debug_invalidate_system_caches_always that has been introduced in v14. > > It can be used to switch off cache invalidation in > > CLOBBER_CACHE_ALWAYS builds which makes cache sensitive tests stable. > > Using this GUC, it is quite possible to make cached connection > > management function tests more meaningful by returning original > > values(true/false, all the output columns) instead of SELECT 1. > > Note that this needs an update in the wake of d68a00391. > > More generally, though, I am not sure that I believe the premise of > this patch. AFAICS it's assuming that forcing debug_discard_caches > off guarantees zero cache flushes, which it does not. (If it could, > we wouldn't need the whole thing; the point of that variable is to > deterministically force flushes which would otherwise be > nondeterministic, not nonexistent.) Can the setting debug_discard_caches = 0 still make extra flushes/discards (not the regular cache flushes/discards that happen because of alters or changes in the cached elements)? My understanding was that debug_discard_caches = 0, disables all the extra flushes with clobber cache builds. If my understanding wasn't right, isn't it good to mention it somewhere in the documentation or in the source code? > Even in a contrib test that > seemingly has nothing else running, background activity such as > autovacuum could result in surprises. So I fear that what you have > got here is a patch that will work 99% of the time; which is not > good enough for the buildfarm. If the setting debug_discard_caches = 0 makes at least a few extra cache flushes, I don't mind withdrawing this patch. Regards, Bharath Rupireddy.
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> On Thu, Jul 15, 2021 at 3:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> More generally, though, I am not sure that I believe the premise of
>> this patch. AFAICS it's assuming that forcing debug_discard_caches
>> off guarantees zero cache flushes, which it does not.
> Can the setting debug_discard_caches = 0 still make extra
> flushes/discards (not the regular cache flushes/discards that happen
> because of alters or changes in the cached elements)? My understanding
> was that debug_discard_caches = 0, disables all the extra flushes with
> clobber cache builds. If my understanding wasn't right, isn't it good
> to mention it somewhere in the documentation or in the source code?
The reason for this mechanism is that cache flushes can be triggered
at any time by sinval messages from other processes (e.g., background
autovacuums). Setting debug_discard_caches allows us to exercise
that possibility exhaustively and make sure that the code is proof
against cache entries disappearing unexpectedly. Not setting
debug_discard_caches doesn't mean that that can't happen, only that
you can't predict when it will happen.
regards, tom lane
On Sat, Jul 17, 2021 at 9:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: > > On Thu, Jul 15, 2021 at 3:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> More generally, though, I am not sure that I believe the premise of > >> this patch. AFAICS it's assuming that forcing debug_discard_caches > >> off guarantees zero cache flushes, which it does not. > > > Can the setting debug_discard_caches = 0 still make extra > > flushes/discards (not the regular cache flushes/discards that happen > > because of alters or changes in the cached elements)? My understanding > > was that debug_discard_caches = 0, disables all the extra flushes with > > clobber cache builds. If my understanding wasn't right, isn't it good > > to mention it somewhere in the documentation or in the source code? > > The reason for this mechanism is that cache flushes can be triggered > at any time by sinval messages from other processes (e.g., background > autovacuums). Setting debug_discard_caches allows us to exercise > that possibility exhaustively and make sure that the code is proof > against cache entries disappearing unexpectedly. Not setting > debug_discard_caches doesn't mean that that can't happen, only that > you can't predict when it will happen. Thanks. I'm fine with dropping this patch, hence I marked the CF entry as "rejected". Regards, Bharath Rupireddy.