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.

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

[YSQL] TupleDesc reference leak for INSERT ON CONFLICT #23429

Closed
1 task done
jasonyb opened this issue Aug 8, 2024 · 0 comments
Closed
1 task done

[YSQL] TupleDesc reference leak for INSERT ON CONFLICT #23429

jasonyb opened this issue Aug 8, 2024 · 0 comments
Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug priority/medium Medium priority issue

Comments

@jasonyb
Copy link
Contributor

jasonyb commented Aug 8, 2024

Jira Link: DB-12350

Description

The first two comments of #6782 show WARNING: TupleDesc reference leak: TupleDesc 0x123ce9eb8 (16541,-1) still referenced type warnings. These are related to estate->yb_conflict_slot and can be fixed separately from #6782.

Issue Type

kind/bug

Warning: Please confirm that this issue does not contain any sensitive information

  • I confirm this issue does not contain any sensitive information.
@jasonyb jasonyb added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels Aug 8, 2024
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels Aug 8, 2024
@jasonyb jasonyb closed this as completed in efd4cb7 Aug 9, 2024
jasonyb pushed a commit that referenced this issue Aug 9, 2024
Summary:
 43d4b49 [PLAT-14771]: Restrict user actions on releases page based on appropriate RBAC permissions
 1473a23 [PLAT-14792]: Master and TServer nodes displays empty tool tip if nodes are unreachable
 Excluded: 78a2bc7 [#23114] YSQL: Refactoring the yb_pg_pgaudit.sql/.out test.
 da9b281 [doc][ybm] Prometheus integration (#23292)
 bd4874b [#13358] YSQL: Fix DDL atomicity stress test failure in tsan build
 edd8e3f [PLAT-14524] Undo all @JsonProperty annotations added earlier to fix APIs
 Excluded: 4a2657e [PLAT-14787] Implement a master tablet lb api for YW
 b9d2e9d [#23445]yugabyted: Node not starting with DNS name and `--secure` option
 f171e13 [#23447]yugabyted: Node doesn't restart with `--secure` enabled
 d4f036d [PLAT-14790] Region metadata is not populated when provisioned nodes are added via Node Agent when using assisted Manual Provisioning (provision_instance.py script)
 71ab66f [doc][ybm] Backup and restore clarifications (#23400)
 4768023 [doc] ysql_yb_bnl_batch_size flag (#23397)
 3d4bc2a [#23117] YSQL: Enable ALTER VIEW in parser
 Excluded: 03bbbed Bumping version to 2.23.1.0 on branch master
 622046d [#23335] DocDB: Set field close timestamp for the log segment being copied
 Excluded: f69b08f [#1999] YSQL: fix temp table INSERT ON CONFLICT
 Excluded: efd4cb7 [#23429] YSQL: fix INSERT ON CONFLICT TupleDesc ref leak
 Excluded: 9e7181f [PLAT-14785] Add REST APIs for job scheduler (auto-master failover)
 d56903c [PLAT-14850]API Token authentication loops through the users and checks token against each of these.

Test Plan: Jenkins: rebase: pg15-cherrypicks

Reviewers: jason, tfoucher

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37201
jasonyb pushed a commit that referenced this issue Aug 13, 2024
…eDesc ref leak

Summary:
MERGE:
efd4cb7

YB pg15 initial merge refers to
55782d5.

- execIndexing.c:
  - function declarations: YB master
    efd4cb7 adds ybConflictSlot
    parameter; upstream PG changes indentation.
- nodeModifyTable.c:
  - function declarations: (same).
  - ExecCheckIndexConstraints function call: YB master
    efd4cb7 adds ybConflictSlot
    parameter; upstream PG a04daa97a4339c38e304cd6164d37da540d665a8
    adds resultRelInfo.
  - ExecOnConflictUpdate function call: YB master
    efd4cb7 adds ybConflictSlot
    parameter; upstream PG 25e777cf8e547d7423d2e1e9da71f98b9414d59e also
    touches parameters.
  - yb_skip_transaction_control_check: YB master
    efd4cb7 changes
    estate->yb_conflict_slot to ybConflictSlot and adds corresponding
    assert.  YB pg15 initial merge flips if and else cases, causing
    merge to get confused.  Carefully apply the same logic formerly in
    the if case now to the else case.
- executor.h:
  - function declarations: (same as execIndexing.c).
- yb_pg_with.out, yb_pg_with.sql:
  - Update a row more than once: YB pg15
    b68a24e adds "YB" to a comment, and
    YB master efd4cb7 deletes that
    comment.  Delete.

Nested INSERT ON CONFLICT causes TupleDesc reference leak warning.  The
cause is obvious: both the outer and inner INSERT ON CONFLICT use the
same estate->yb_conflict_slot:

1. outer: set estate->yb_conflict_slot
2. inner: set estate->yb_conflict_slot
3. inner: free estate->yb_conflict_slot
4. outer: free estate->yb_conflict_slot

The slot allocated in (1) is not freed.

Fix by removing yb_conflict_slot and using local variable
ybConflictSlot and passing it through functions like PG's conflictTid.
In future PG versions, resultRelInfo is local to the node, so maybe this
can be put into resultRelInfo to reduce modifications to the functions
signatures and make future merges easier.

Add test coverage using examples from issue #6782.
Jira: DB-12350

Test Plan:
On Almalinux 8:

    ./yb_build.sh fastdebug --gcc11 --java-test TestPgRegressPgMisc
    ./yb_build.sh fastdebug --gcc11 --java-test 'TestPgRegressMisc#testPgRegressMiscSerial4'

Jenkins: rebase: pg15-cherrypicks

Reviewers: kramanathan, aagrawal

Reviewed By: aagrawal

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37248
@yugabyte-ci yugabyte-ci removed the status/awaiting-triage Issue awaiting triage label Aug 21, 2024
@github-project-automation github-project-automation bot moved this to Done in YSQL Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug priority/medium Medium priority issue
Projects
Status: Done
Development

No branches or pull requests

2 participants