Fix alter_job not updating new job fields #2404
Conversation
svenklemm
requested review from
pmwkaa,
k-rus and
gayyappan
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## master #2404 +/- ##
==========================================
+ Coverage 89.95% 90.09% +0.13%
==========================================
Files 213 213
Lines 34354 34323 -31
==========================================
+ Hits 30904 30923 +19
+ Misses 3450 3400 -50
Continue to review full report at Codecov.
|
svenklemm
added
bgw
There was a problem hiding this comment.
LGTM, few nit comments
Can you add commit message body and clarify, which new job fields are fixed? Isn't just the config in JSON?
Can you add link to the issue being fixed?
Variable name mod_tuple is not easy to follow, so I suggest to fix it back to new_tuple, which is commonly used through the code.
It will be an advantage of having new line separating declaration block and the assert.
| @@ -907,21 +907,25 @@ | |||
| BgwJob *updated_job = (BgwJob *) data; | |||
| bool should_free; | |||
| HeapTuple tuple = ts_scanner_fetch_heap_tuple(ti, false, &should_free); | |||
| HeapTuple new_tuple = heap_copytuple(tuple); | |||
| FormData_bgw_job *fd = (FormData_bgw_job *) GETSTRUCT(new_tuple); | |||
| HeapTuple mod_tuple; | |||
| TimestampTz next_start; | |||
|
|
|||
There was a problem hiding this comment.
Since the following lines belong to the variable declaration block it can be valuable to remove this new line.
| Datum values[Natts_bgw_job] = {0}; | ||
| bool isnull[Natts_bgw_job] = {0}; | ||
| bool repl[Natts_bgw_job] = {0}; | ||
|
|
| slot_getattr(ti->slot, Anum_bgw_job_schedule_interval, &isnull[0]); | ||
| Assert(!isnull[0]); |
There was a problem hiding this comment.
New line will be an advantage here:
| slot_getattr(ti->slot, Anum_bgw_job_schedule_interval, &isnull[0]); | |
| Assert(!isnull[0]); | |
| slot_getattr(ti->slot, Anum_bgw_job_schedule_interval, &isnull[0]); | |
| Assert(!isnull[0]); |
src/bgw/job.c
Outdated
| @@ -907,21 +907,25 @@ bgw_job_tuple_update_by_id(TupleInfo *ti, void *const data) | |||
| BgwJob *updated_job = (BgwJob *) data; | |||
| bool should_free; | |||
| HeapTuple tuple = ts_scanner_fetch_heap_tuple(ti, false, &should_free); | |||
| HeapTuple new_tuple = heap_copytuple(tuple); | |||
| FormData_bgw_job *fd = (FormData_bgw_job *) GETSTRUCT(new_tuple); | |||
| HeapTuple mod_tuple; | |||
There was a problem hiding this comment.
mod_tuple is confusing name. I recommend to use the original name new_tuple, which is common name in all places in our code.
There was a problem hiding this comment.
newtuple is also common for PG code.
svenklemm
force-pushed
the
fix_alter_job
branch
2 times, most recently
from
86039f1
to
9a04cfe
Compare
Fix bgw_job_tuple_update_by_id to also update scheduled and config field of bgw_job.
svenklemm
force-pushed
the
fix_alter_job
branch
from
9a04cfe
to
c8c60b5
Compare
Fix bgw_job_tuple_update_by_id to also update scheduled and config
field of bgw_job.
Fixes #2402