Fix segfault after second ANALYZE

[sb230132]

Issue occurs in extended query protocol mode only where every query goes through PREPARE and EXECUTE phase. First time ANALYZE is executed, a list of relations to be vaccumed is extracted and saved in a list. This list is referenced in parsetree node. Once execution of ANALYZE is complete, this list is cleaned up, however reference to the same is not cleaned up in parsetree node. When second time ANALYZE is executed, segfault happens as we access an invalid memory location.

Fixed the issue by restoring the actual value in parsetree node once ANALYZE completes its execution.

Fixes #4857

[codecov]

Codecov Report

Merging #5054 (0910f45) into main (d927390) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5054      +/-   ##
==========================================
- Coverage   89.61%   89.61%   -0.01%     
==========================================
  Files         227      227              
  Lines       51608    51609       +1     
==========================================
- Hits        46251    46247       -4     
- Misses       5357     5362       +5     

Impacted Files	Coverage Δ
src/process_utility.c	90.20% <100.00%> (+<0.01%)	⬆️
src/loader/bgw_launcher.c	89.51% <0.00%> (-2.55%)	⬇️
src/bgw/job_stat.c	91.87% <0.00%> (-0.32%)	⬇️
tsl/src/bgw_policy/job.c	88.31% <0.00%> (-0.05%)	⬇️
src/bgw/scheduler.c	85.16% <0.00%> (+1.27%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d927390...0910f45. Read the comment docs.

[jnidzwetzki]

I am curious, do you know in which places stmt->rels is changed? Is there any change besides the one in line 963 (stmt->rels = list_concat(ctx.chunk_rels, vacuum_rels);).

Maybe we could also fix the problem by creating a new list there and using it for the comparision a few lines later. Then we would save the copy and restore operation and stmt->rels would always contain valid entries.

[sb230132]

You are right. We can fix it either way.
I tried the same approach initially. I created a new list instead of modifying stmt->rels, but i could not pass this new list to ExecVacuum(), as this API expects VacuumStmt as 2nd parameter. Which in turn requires me to create a new VacuumStmt object and set the new list inside it.

Thus i took alternative approach of saving the original list, and restoring the saved list.

[fabriziomello]

@sb230132 isn't this issue similar to 74ca546 ?

[sb230132]

@fabriziomello Issue you pointed out is same as this one. I did try to allocate a new memory context for stmt rel list and free it up, however during completion of this function process_vacuum() i need to restore the stmt rel list. As changes to this data structure should not be visible to the caller of this function.
Something similar is done in src/backend/commands/vacuum.c/vacuum(). Stmt rel list which is passed to vacuum() is pass by value, thus any changes done inside vacuum() is lost once this function call execution is done.

Thanks for pointing to this issue. It was more informative.

[fabriziomello]

@fabriziomello Issue you pointed out is same as this one. I did try to allocate a new memory context for stmt rel list and free it up, however during completion of this function process_vacuum() i need to restore the stmt rel list. As changes to this data structure should not be visible to the caller of this function. Something similar is done in src/backend/commands/vacuum.c/vacuum(). Stmt rel list which is passed to vacuum() is pass by value, thus any changes done inside vacuum() is lost once this function call execution is done.

Didn't get your point. Where exactly Postgres do it in src/backend/commands/vacuum.c:vacuum()? The problem here is subsequent executions of an utility command. The first execution is prepared/cached (extended protocol / plpgsql do it) but the changed relation list is allocated in other memory context, so the subsequent execution will segfault due to memory violation. Am I missing something here?

[sb230132]

@fabriziomello Issue you pointed out is same as this one. I did try to allocate a new memory context for stmt rel list and free it up, however during completion of this function process_vacuum() i need to restore the stmt rel list. As changes to this data structure should not be visible to the caller of this function. Something similar is done in src/backend/commands/vacuum.c/vacuum(). Stmt rel list which is passed to vacuum() is pass by value, thus any changes done inside vacuum() is lost once this function call execution is done.

Didn't get your point. Where exactly Postgres do it in src/backend/commands/vacuum.c:vacuum()? The problem here is subsequent executions of an utility command. The first execution is prepared/cached (extended protocol / plpgsql do it) but the changed relation list is allocated in other memory context, so the subsequent execution will segfault due to memory violation. Am I missing something here?

You are right.
Let me give more description:
Check below function:

void
ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)

In this function there is a call to

vacuum(vacstmt->rels, &params, NULL, isTopLevel);

vacstmt->rels hold list of relations which needs to be vaccumed.
ex:
ANALYZE t1,t2,t2;
vacstmt->rels will have 3 entries representing t1,t2,t3

ANALYZE;
In this case vacstmt->rels has 0 entries.

When timescaledb extension is installed.
First time ANALYZE is executed:

1. In src/process_utility.c::process_vacuum() call, stmt->rels is filled with list of relations which needs to be vacuumed.
2. This list is then passed on to ExecVacuum().
3. Once process_vacuum() is finished execution the list which holds all relations to be vacuumed is freed up.

Second time ANALYZE is executed:

1. In src/process_utility.c::process_vacuum(), stmt->rels has the same list which has been deallocated.
2. Same list is accessed in ExecVacuum() and thus causes a crash.
3. Thus we need to reset stmt->rels everytime at end of process_vacuum() execution.

[jnidzwetzki]

One comment, rest looks good to me.