-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove CacheInvalidateRelcacheByTuple and put CronJobCacheValid in shmem #295
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree [company="{your company}"] |
@microsoft-github-policy-service agree |
src/job_metadata.c
Outdated
CachedCronJobRelationId = InvalidOid; | ||
} | ||
if (!pg_atomic_unlocked_test_flag_impl(CronJobCacheValid)) | ||
pg_atomic_clear_flag_impl(CronJobCacheValid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't quite follow this approach. Shared memory does not follow transaction boundaries (we will see the valid flag become false before commit), whereas invalidations do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't quite follow this approach. Shared memory does not follow transaction boundaries (we will see the valid flag become false before commit), whereas invalidations do.
Yes, I have dived into the invalidation method, when the transaction commited, the pg_cron laucher can receive the invalidation messages and call 'InvalidateJobCacheCallback' to set the 'CronJobCacheValid' to be false. And when the transaction is aborted, pg_cron launcher cannot receive any messages.
I agree that using the flag in shared memory can not follow transaction boundaries, although I can use a map to store the 'transactionID' and the ' CronJobCacheValid' value for each transaction and do something in at then end of the transaction, it looks a little complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcoslot I add a new commit in this pr, using xact_callback to catch table change event
There is no need to call `CacheInvalidateRelcacheByTuple` when only updating the content of cron.job table( `CacheInvalidateRelcacheByTuple` should be called when the schema of crom.job udpated). Each time the `CacheInvalidateRelcacheByTuple` is called, the `Relaion` of pg_cron in launcher will be rebuilt which can lead some overhead. Instead, put `CronJobCacheValid` into shmem, and set it to false in function InvalidateJobCache.
Register the xact_callback function pgcron_xact_callback to record if the data in the data has changed. when transaction commits or aborts, reset the JobTableChanged when transaction commits, if the JobTableChanged is true, set CronJobCacheValid to false. One drawback of this method is that in two_phase transaction, if the transaction rollback, it will still be treat as commited as there is no such transaction event for it in postgres now.
There is no need to call
CacheInvalidateRelcacheByTuple
when only updating the content of cron.job table(CacheInvalidateRelcacheByTuple
should be called when the schema of crom.job udpated). Each time theCacheInvalidateRelcacheByTuple
is called, theRelaion
of pg_cron in launcher will be rebuilt which can lead some overhead.Instead, put
CronJobCacheValid
into shmem, and set it to false in function InvalidateJobCache.