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][Upgrade] Direct DMLs on system tables should update system cache #13500

Open
frozenspider opened this issue Aug 1, 2022 · 3 comments
Open
Assignees
Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug priority/medium Medium priority issue

Comments

@frozenspider
Copy link
Contributor

frozenspider commented Aug 1, 2022

Jira Link: DB-3096

Description

When doing direct INSERT/UPDATE/DELETE on system tables, related PG system caches are only updated on INSERT path.

@frozenspider frozenspider added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels Aug 1, 2022
@frozenspider frozenspider self-assigned this Aug 1, 2022
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels Aug 1, 2022
@frozenspider frozenspider changed the title [YSQL][Upgrade] Direct DMLs on system tables do not update system cache [YSQL][Upgrade] Direct DMLs on system tables should update system cache Aug 1, 2022
@yugabyte-ci yugabyte-ci removed the status/awaiting-triage Issue awaiting triage label Aug 8, 2022
jayant07-yb pushed a commit to jayant07-yb/yugabyte-db that referenced this issue Dec 7, 2022
…pdate system cache

Summary:
When issuing direct DMLs to system tables (whether in an upgrade more or not), PG system caches are only updated for `INSERT` but not for `UPDATE`/`DELETE`. This is due to the fact that code path for `UPDATE`/`DELETE` was different for regular and system tables (latter were only called internally by DDLs).
This diff changed DML `UPDATE`/`DELETE` with catalog tuple invalidation to align with DDLs. (Full code path unification was discussed but left out of the scope of this diff)

Additionally:
* Optimized arguments for internal YB DML pggate functions.
* Renamed `yb_mt_is_single_row_update_or_delete` to `yb_fetch_target_tuple` (with inverted meaning) to avoid ambiguity with `yb_es_is_single_row_modify_txn`

Other changes are minor.

---

Resolves yugabyte#13500

Test Plan: ybd --java-test 'org.yb.pgsql.TestYsqlUpgrade#dmlsUpdatePgCache'

Reviewers: mihnea, tverona, myang, jason, amartsinchyk

Reviewed By: amartsinchyk

Subscribers: amartsinchyk, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D18667
@frozenspider
Copy link
Contributor Author

Turns out cache update didn't work when oid was filtered from HeapTuple by a junk filter.

CREATE OR REPLACE FUNCTION inc(i integer) RETURNS integer AS $$
  BEGIN RETURN i + 1; END;
$$ LANGUAGE plpgsql;

BEGIN;
UPDATE pg_proc SET proargnames = '{j}' WHERE proname = 'inc';
CREATE OR REPLACE VIEW inc_view AS SELECT inc(j => 10);
-- ERROR: function inc(j => integer) does not exist
--  Hint: No function matches the given name and argument types. You might need to add explicit type casts.

@frozenspider frozenspider reopened this Dec 12, 2022
fizaaluthra added a commit that referenced this issue Feb 2, 2023
Summary:
D20365 introduced flakiness in `TestYsqlUpgrade`. This is caused due to GH issue #13500.
As a work-around, change the migration introduced in D20365 to use a `DELETE` followed by `INSERT` instead of `UPDATE` on `pg_proc` (since the PG system caches are correctly updated on `INSERT` path)

Also fix the additional failures in `TestYsqlUpgrade#upgradeIsIdempotent`, `TestYsqlUpgrade#upgradeIsIdempotentSingleConn` and `migratingIsEquivalentToReinitdb` introduced by D22690.

Test Plan:
Jenkins: test regex: (.*TestYsqlUpgrade.*)|(.*YbAdminSnapshotSchedule.*)

./yb_build.sh  --java-test 'org.yb.pgsql.TestYsqlUpgrade'

Reviewers: jason, tverona

Reviewed By: tverona

Differential Revision: https://phabricator.dev.yugabyte.com/D22690
@yugabyte-ci yugabyte-ci assigned tverona1 and unassigned frozenspider Feb 28, 2023
karthik-ramanathan-3006 added a commit that referenced this issue Aug 16, 2023
Summary:
D26677 introduced the yb_backend_xid column to pg_stat_get_activity.
The corresponding migration script UPDATES the existing row for pg_stat_get_activity to add the new column.
This seemingly causes flakiness during upgrades owing to the fact that the system table update command does not update the local cat cache.
Subsequent to updation of the local cat cache, the views that are based on pg_stat_get_activity are recreated. During this process, the out of date cat cache throws an error that the new column is not found.
To mitigate this, this revision replaces the UPDATE with a DELETE + INSERT.

For long term tracking of the UPDATE issue please refer to GHI [[ #13500 | #13500 ]].

This revision also deletes a redundant operation wrt creating views in the same migration script.
Jira: DB-7569

Test Plan:
Jenkins: all tests

1. Run upgrade tests:
```
./yb_build.sh --java-test 'org.yb.pgsql.TestYsqlUpgrade'
```

2. Perform portal upgrades to latest master, as well as 2.19.X.

Reviewers: jason, tverona, fizaa, myang

Reviewed By: myang

Subscribers: yql, smishra

Differential Revision: https://phorge.dev.yugabyte.com/D27722
@fizaaluthra
Copy link
Member

Previously the upgrade failures due to this issue were hard to reproduce locally (see migrations V43, V31), but now these can be reproduced consistently with ysql_enable_read_request_caching set to true.

@yifanguan
Copy link
Contributor

I have an in-progress diff which partially solves this problem (no mixed DDL & DML in the same txn).
However, for alex's example above (direct DML + cache usage within a same txn), I think cache invalidation works properly. We seem to fetch old data from DocDB while loading PG system cache. My hypothesis is CREATE VIEW starts its own YB DDL txn and cannot see uncommitted UPDATE change even if they are within the same "txn".

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
None yet
Development

No branches or pull requests

5 participants