Skip to content
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.

Sign up for GitHub

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

Add missing lock to Constraint-aware append #7515

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add missing lock to Constraint-aware append #7515

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/nodes/constraint_aware_append/constraint_aware_append.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <optimizer/plancat.h>
#include <parser/parsetree.h>
#include <rewrite/rewriteManip.h>
#include <storage/lockdefs.h>
#include <utils/lsyscache.h>
#include <utils/memutils.h>
#include <utils/syscache.h>
Expand Down Expand Up @@ -78,6 +79,8 @@ can_exclude_chunk(PlannerInfo *root, EState *estate, Index rt_index, List *restr
{
RangeTblEntry *rte = rt_fetch(rt_index, estate->es_range_table);

/* Need AccessShareLock to call excluded_by_constraint() */
LockRelationOid(rte->relid, AccessShareLock);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that it's not locked in this place looks like a bug. All RangeTableEntries are supposed to be locked in AccessShareLock mode very early, in upstream inheritance this is done in expand_inherited_rtentry() -> find_inheritance_children(). Did we somehow miss the lock in our chunk expansion code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks like it. You might be right that this could be a bigger issue with our expansion code so I will dig a bit deeper to see what I can find.

return rte->rtekind == RTE_RELATION && rte->relkind == RELKIND_RELATION && !rte->inh &&
excluded_by_constraint(root, rte, rt_index, restrictinfos);
}
Expand Down
Loading