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

Test fixes #65

Open
wants to merge 37 commits into
base: pg15
Choose a base branch
from
Open

Test fixes #65

wants to merge 37 commits into from

Conversation

arpang
Copy link
Owner

@arpang arpang commented Sep 29, 2023

No description provided.

nocaway and others added 30 commits July 20, 2023 02:08
Summary: Merge with PG13

Test Plan:
None

Suppressed compiling Postgres's extension. Removed obsolete Postgres's types from docdb
…hecking the condition for creating a primary key index

Summary:
The attribute `yb_is_add_primary_key` was created as part of the `AlterTableCmd` struct to indicate this alter command is for adding primary key.
In the past, there were 2 subcommands involved when creating a primary key: 1) `AddIndex` 2) `SetNotNull` (only if any one of the primary key attributes are not set as NOT NULL).
In PG13, setting the primary key columns to NOT NULL is triggered in a separate API called `transformIndexConstraint`.
The boolean flag has been set in the corresponding section where `AlterTableCmd` is prepared to alter the primary key column to be not null.

Here is the excerpt from the PG13 comment mentioning the need to satisfy NOT NULL requirement prior to creating primary key index:
```Now we expect that the parser inserted any required ALTER TABLE SET NOT NULL operations before trying to create a primary-key index.```

Test Plan: Jenkins

Reviewers: neil, myang

Reviewed By: myang

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D21331
Summary:
Fix two compilation errors:

- release build got -Wsometimes-uninitialized compilation error because of
  Assert(false) that is treated differently in release/debug builds
- gcc build got -Werror=comment because of bad comment

Test Plan:
Jenkins: compile only, rebase: pg13

    ./yb_build.sh fastdebug --gcc11 --no-initdb
    ./yb_build.sh release --clang15 --no-initdb

Reviewers: smishra, neil

Reviewed By: neil

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D22577
Summary:
YbClearCurrentTransactionId() was introduced to handle temp tables created during concurrent
materialized view refreshes correctly.
PG13 no longer uses `CurrentTransactionState->transactionId` and `MyPgXact`. This diff changes the
code to use the PG13 equivalent of these fields/structures -
`CurrentTransactionState->fullTransactionId` and `MyProc->xid`.

Test Plan: Jenkins: skip

Reviewers: neil, smishra

Reviewed By: neil, smishra

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D21721
Summary:
When migrating from PG 11.2 to PG 15, we come across commit
49fa99e54ec0ded180b52a4a14e543702d53e8c9 that makes make_greater_string
private:

    Move pattern selectivity code from selfuncs.c to like_support.c.
    ...
    This change localizes the use of pattern_fixed_prefix() and
    make_greater_string() so that they no longer need be exported.
    (We might get pushback from extensions about that, perhaps,
    in which case I'd be inclined to re-export them in a new header
    file like_support.h.)
    ...

ybgin uses make_greater_string for tsvector prefix logic.  Since it is
not trivial to indirectly get the benefit of make_greater_string through
the publicly exposed support functions (they take complex Node
argument), lean towards re-exposing the function through new header
yb_like_support.h and new function yb_make_greater_string.  If, in the
future, upstream postgres decides to expose make_greater_string, these
changes can be scrapped in favor of using make_greater_string again.

Test Plan: Jenkins: compile only, rebase: pg15

Reviewers: neil, amartsinchyk

Reviewed By: amartsinchyk

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D23110
Summary:
build-support/jenkins/build-and-test.sh involves running doctest on
select python files.  This is what jenkins runs, whereas developers
typically run ./yb_build.sh.

doctest appears to have a strange bug where it does not like it when
sys.stdout is set to something else by the time of the end of file.
This doesn't seem to matter when executing the python script itself: it
only matters for doctest.  PG 15 merge involves updating
generate_unaccent_rules.py, and this update hits the above bug.  Appease
doctest by restoring sys.stdout to the original object at the end of the
file.

It seems wasteful to run doctest on files that don't have docstrings,
but that can be fixed in the future.

Test Plan: jenkins: compile only, rebase: pg15

Reviewers: mbautin, neil, steve.varnau

Reviewed By: steve.varnau

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D23266
Summary:
    In the master branch, for memory tracking, we added `oldsize` for the block.
    It overwrites the variable with the same name in the outer scope, defined for the chunk.
    However, in PG upstream, the `oldsize` in the outer scope is referenced in the inner scope.
    Because of the naming overwritting, after merging, the reference used becomes incorrect.

    **Solution**
    I basically "backport" how the upstream creates the variable for "block old size" and use
    it is for YB memory tracking.

    Along with this, it also fixes two potential overflow issues in the existing code.
    The fixes are needed in the master as well. So a backport to the PG13 branch will be committed later
    in a separate diff.

Original commit: 83c6b6a/D21128

Test Plan: Jenkins: rebase: pg15

Reviewers: neil, mbautin, telgersma

Reviewed By: telgersma

Subscribers: smishra, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D24000
Fixed compilation errors in /access, /bootstrap, /catalog, and /commands

Fixed compilation errors in /parser

Fixed compilation errors in /commands

Fixed compilation errors in /executor

Fixed compilation errors in /libpq

Fixed compilation errors in /optimizer

Fixed compilation errors in /utils

Completed fixing errors for directories in /backend/*

Commented out Yugabyte's code for tools in src/bin - Revisit later

Add missing files in contrib

Add missing files in postgres/doc
Fixed compilation errors in src/backend

Fixed compilation errors in pg_dump

Used Pg15 structure for FunctionCallInfo

Remove Yugabyte version of pull_varattnos_walker() to avoid duplicating code with Pg15.

Activate Yugabyte cluster-connecting code in InitPostgres
Summary: Adds missing oid columns and indexes to YB system tables

Test Plan: N/A

Reviewers: mihnea, neil

Reviewed By: neil

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D24959
Summary: Initdb bootstrap step working

Test Plan: Jenkins: skip

Reviewers: mihnea, neil

Reviewed By: neil

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D25090
Summary: Initdb fixes till setup_collation()

Test Plan: Jenkins: skip

Reviewers: neil, mihnea

Reviewed By: neil

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D25341
Summary: With this diff, initdb is working end to end except for pg_stat_statements.

Test Plan: Jenkins: rebase: no

Reviewers: neil, mihnea

Reviewed By: neil

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D25787
Summary:
This diff addresses the below issues/fixes:

  # Uncomments python doctest failures fix: https://phorge.dev.yugabyte.com/D23266
  # PG changed default value of pg_class.reltuples from 0 to -1: postgres/postgres@3d351d9
  # Fixes index scan
  # IndexQualInfo is removed and IndexClause is introduced
  # ModifyTablePath has subpath (a single source data path) instead of a list
  # Fixes insert queries which did not have values for all the columns.
  # Build failure cause due to missing arg in InstrAlloc
  # Create table failure FailedAssertion("ptr == NULL || nodeTag(ptr) == type")
  # Single row update optimization (yb_single_row_update_or_delete_path)

Consider table: create table emp(id int primary key, name text). Queries which were not working/working incorrectly before this diff that has been fixed:

  # insert into emp values (1), (2), (3);
  # select * from emp where id = 1; (this was working but was taking foreign scan path and not index scan)
  # delete from emp where id = 1;
  # delete from emp where id >= 1;
  # update emp set name = 'John' where id = 1;
  # update emp set name = 'John' where id >= 1;
  # update my_emp set id = id + 4 where id = 1; (point PK update)
  # update my_emp set id = id + 4 where id > 1; (range PK update)
  # Tested updates with before update triggers
  # Tested updates with pushdown (took test from yb_dml_single_row.sql)

Test Plan: Jenkins: rebase: no

Reviewers: tnayak, smishra, neil

Reviewed By: tnayak

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D27692
Test Plan:
Jenkins: rebase: no
./yb_build.sh --java-test org.yb.pgsql.TestPg15Regress#testPg15Regress

Reviewers: smishra, neil

Reviewed By: smishra

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D27769
Summary:
Summary of the changes:

  # PG15 commutes `RestrictInfo.clause` if the variable is on the right (for instance, in query `select * from emp where 1 = id;`). For implementation, search `commrinfo = commute_restrictinfo(rinfo, comm_op);` in `indxpath.c`. As a consequence function `deconstruct_indexquals` and struct `IndexQualInfo` have been removed. This diff handles this change for YB.

  # Instead of maintaining `is_hashed` state (`IndexQualInfo` is removed), it is converted into a helper function. See `is_hashed` in `yb_scan.c`. This is open for review.

  # Handles type change of `ReservoirStateData.randstate` from array to a struct of two `uint64`s.

Test Plan: Jenkins: rebase: no

Reviewers: smishra, mihnea, neil, tnayak

Reviewed By: tnayak

Subscribers: yql, ybase

Differential Revision: https://phorge.dev.yugabyte.com/D27737
Summary:
This diff fixes the following:

  # Enables the code block in `ClientAuthentication` in `auth.c`, which was disabled during the merge. A minor code modification is done related to the oid now being a regular column.
  # Enables code blocks in `hba.c` which were disabled during the merge. Previously, a portion of `tokenize_file` was refactored out into a common helper function `tokenize_line` for PG/YB. Subsequent changes in PG upstream led to conflicts during the merge. This diff retains the PG upstream code as it is, along with a YB-specific function `yb_tokenize_line`. It also renames `tokenize_hardcoded` to `yb_tokenize_hardcoded` as it YB-specific.
  # Adds missing case for `T_BackfillIndexStmt` in the function `ClassifyUtilityCommandAsReadOnly.`
  # Removes ` Assert(0);` from function `IndexBackfillHeapRangeScan`. YB_TODO comment related to handling the `progress` parameter is already present in the function; hence, assert(0) is not required.

With fixes #2, #3, and #4, `CREATE INDEX CONCURRENTLY` works on pg15 branch.

Test Plan: Jenkins: rebase: no

Reviewers: smishra, neil, jason

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D27860
Summary:
This diff fixes the optimizer and executor directory.

  # In pg15, oid column is treated like a regular column, hence made `ybFetchNext` return virtual tuple table slot always (see function comment for more context)

  # `systable_recheck_tuple` doesn't work in YB. Previously, we disabled calling it in YB. In the newer PG version, new calls to this function were introduced and that caused issues. This can happen in the future versions as well. Hence, I refactored the function itself to return without executing it in YB.

  # PG introduced support for collation in the hashing functions (postgres/postgres@5e1963f). This diff disables it for YB (this is open for review).

  # Enabled the code blocks disabled during the merge related to YB batched nested loop join.
  # Removes unused OID returned from YB modify functions (insert, update).
  # Fixes related to single row modify.

Test Plan: Jenkins: rebase: no

Reviewers: neil, tnayak

Reviewed By: tnayak

Subscribers: smishra, yql

Differential Revision: https://phorge.dev.yugabyte.com/D27766
Summary:
This diff fixes the todos in catalog directory:

  # Removes `Assert(!shared_relation);` from `heap_create_with_catalog` function as pg15 creates entries in pg_type for array types of shared relations as well.
  # Cleans up functions that have been removed from upstream pg: `GetNewOid`, `RemoveCollationById`, `RemoveConversionById`, `isSharedObjectPinned`.
  # Discussion related to `pg_stat_progress_create_index ` changes in `system_views.sql`: https://yugabyte.slack.com/archives/C03L0TLANHY/p1689070967517019
  # Changes related to pg15 upstream replacing explicit object pinning with range test: postgres/postgres@a49d081 (One more pass is required to handle this change for YB. I will do it as part of a subsequent diff. In this diff, I am only cleaning up YB-specific function calls, which are anyway deprecated / unusable and hence were disabled during merge).

Test Plan: Jenkins: rebase: no

Reviewers: neil, yguan, fizaa

Reviewed By: yguan

Subscribers: mihnea, yql

Differential Revision: https://phorge.dev.yugabyte.com/D27741
Summary:
This diff fixes the following issues in the utils directory:

  # Adds missing YB functions in pgstat.c.
  # Removes special handling of oid in relcache.c.
  # Removes function `assign_pgstat_temp_directory` from guc.c, which was removed from PG upstream.
  # PG has introduced an optimization to use generation to allocate memory context. This was causing merge join to fail in YB. Disabled it for now.

Test Plan: Jenkins: rebase: no

Reviewers: smishra, neil, tvesely

Reviewed By: tvesely

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D27985
Summary: Fix clangd indexing if the DLSUFFIX definition does not appear in compiler commands.

Test Plan:
Jenkins: skip

./yb_build.sh --cio
bin/run_codecheck

Reviewers: tfoucher

Reviewed By: tfoucher

Subscribers: ybase

Differential Revision: https://phorge.dev.yugabyte.com/D28478
Summary:
This diff fixes the following runtime issues. Except for one (highlighted below), these issues have existed since the initial merge of YB with PG15 changes:

in tablecmds.c
# Modifies macro `YBGetNotNullConstraintAttr` to read `oid` from the `Form_pg_constraint` as `HeapTupleGetOid` is unavailable in PG15.
# PG (in [[ postgres/postgres@e1551f9 | commit ]] ) replaced `AttrNumber *` with `AttrMap *` and `convert_tuples_by_name_map` with `build_attrmap_by_name`. To compile the code after the YB and PG15 merge, YB-specific functions/code block using `AttrNumber *` and `convert_tuples_by_name_map` were disabled. This diff fixes it.
# During the merge commit ([[ yugabyte@a6974e5#diff-551494d846d56417f70e5a060c2a8f90ba65c43019ab7bfdc2c84b109c83f947 | Merge with some of Postgres 15 ]]), `YbHeapTupleGetOid` and `YB_HACK_INVALID_OID` were introduced as hacks to compile the pg15 branch (both of these are `-1`). Some of these usages are removed/replaced with correct OIDs in this diff.
# PG (in [[ postgres/postgres@5815696 | commit ]] ) replaced function `addRTEtoQuery` with `addNSItemToQuery`. To compile the code after the YB and PG15 merge, YB-specific function calls to `addRTEtoQuery` were disabled. This diff fixes this.
# Modifies function `YbATCopyPolicyObjects` to generate new oid while cloning policies using `GetNewOidWithIndex` as is done in `CreatePolicy`.

in indexcmds.c
  # Re-merged the `DefineIndex` function. This function is not merged properly since the rebase commit [[ yugabyte@06d9200 | Rebase with yb::master up to commit 61302b0 ]].

in dbcommands.c
  # Fixes cache reference leak on db creation. This was a bug introduced during the initial merge commit ([[ yugabyte@7e541a1#diff-e825bdb435bd1205f1cdb2db105299d8bafd7c0a8f8e4d7bd7790ca87deff582 | Complete merging with Postgres 15.2 ]]).

checkpointer.c:
  # `DROP DATABASE` was broken at `RequestCheckpoint` function call. I am unaware when did it start breaking. I am also unaware why it doesn't fail on YB master branch. The function (RequestCheckpoint) is not applicable in YB because it doesn't use PG WAL. Hence, disabled this function for YB.

in tablegroup.c
  # Removes a done YB_TODO comment. This was a verification comment. I verified that the values are populated correctly and required no code changes.

in reloptions.c
  # PG (in [[ postgres/postgres@1bbd608 | commit ]]) introduced separate handling of reloptions for partitioned tables via function `partitioned_table_reloptions`. This change in YB was causing partitioned table creation to fail with `FailedAssertion("numoptions <= num_relopt_elems")`. This was because in PG, there no options for partitoned tables but in YB, there is `colocation_id` option. The diff handles it.

in nodeModifyTable.c:
  # Fixes `relcache reference leak` on insertion into a table with secondary indexes by not re-opening already open indices. This issue was introduced during initial YB and PG15 merge commit ([[ yugabyte@a6974e5#diff-7298d5c117fa9b5b7b8642512d380ab43b432a2d2216d4f2ef23096dfac0c921 | "Merge with some of Postgres 15" ]]).
  # PG in [[ postgres/postgres@25e777c#diff-08af8c53c4fe685f813673454d48447222d490bccd75b0c88f2f6a9b83f66fab | commit ]] split ExecUpdate and ExecDelete. This commit was introduced to YB as commit [[ 7e541a1 | Complete merging with Postgres 15.2 ]]. Consequently, this introduced a  bug where secondary index entries were not deleted for DELETE statements. This diff fixes it.

ri_triggers.c:
  #  [[ yugabyte@3312964#diff-9d0e6cee391e5362dc643eecdac580f7d2d0c6c378ad0ff1b35bf4592c388b2f | Utils fixes ]] commit introduced a regression by freeing up `HeapTuple` `new_row` too soon causing FK violation error on insertion even when there are no violations. The diff fixes the bug.

in matview.c
  # frees up `HeapTuple tuple`, if `shouldFree` is set to true by `ExecFetchSlotHeapTuple`.

Test Plan: Jenkins: skip, rebase: pg15

Reviewers: jason

Reviewed By: jason

Subscribers: fizaa, mihnea, smishra, dmitry, ybase, yql

Differential Revision: https://phorge.dev.yugabyte.com/D28432
Summary:
This diff fixes the following:
  # Cleans up (disabled) blocks of code no longer applicable pg15.
  # Renames `NEIL_OID` and `NEIL_TODO` to `YB_TODO` for better tracking.
  # `ObjectIdAttributeNumber` is not available in upstream PG15 as OID is a regular column henceforth. It is retained in YB's pg15 branch as a hack to compile the code till all the references to it are removed. This diff replaces some of the references to `ObjectIdAttributeNumber` with actual OIDs.
  # `kObjectId` is not available in upstream PG15 as OID is a regular column henceforth. This diff cleans the variable along with all of its usage.

Test Plan: Jenkins: skip, rebase: pg15

Reviewers: smishra, jason

Reviewed By: jason

Subscribers: yql, ybase

Differential Revision: https://phorge.dev.yugabyte.com/D28840
Summary:
This diff fixes the following:

  # Reworked `CopyFrom` function as it was not merged properly due to conflicts.
  # Fixed `DoCopyTo` by switching out of temporary memory context - this was mistakenly skipped during the merge. The unused variable `oldcontext` was causing a build failure.
  # Cleanup: `PutMemoryContextsStatsTupleStore` and `pg_get_backend_memory_contexts ` were moved to `mcxtfuncs.c` but the original was not removed from `mcxt.c` during the merge. The unused functions were causing a build failure.

Test Plan:
Jenkins: rebase: pg15, test regex: org.yb.pgsql.TestPgRegress.*

Added copy from test to temporary pg15 test suite. Note that it involves creating a partitioned table that needs D28432.
Also, locally tested the diff with regression test `yb_feature_copy`.

Reviewers: tvesely

Reviewed By: tvesely

Subscribers: dsrinivasan, mihnea, yql

Differential Revision: https://phorge.dev.yugabyte.com/D28500
Summary:
This diff fixes the following:

# PG ( in [[  postgres/postgres@c2fe139 | commit ]]) replaced `heap_beginscan` with `table_beginscan` and `HeapTuple` with `TupleTableSlot` in `validateForeignKeyConstraint`. This was not merged properly with YB and is handled in this diff.
# PG (in [[ postgres/postgres@4c9e1bd | commit ]]) resets memory context once per tuple in `validateForeignKeyConstraint`. This change was not merged properly during the YB and PG15 merge. This diff handles it.
# PG (in [[ postgres/postgres@d6f96ed | commit ]]) added a new feature "Allow specifying column list for foreign key ON DELETE SET actions". The corresponding change to `YbATCreateSimilarForeignKey` is done in this diff.
# PG (in [[ postgres/postgres@f56f8f8#diff-d373fb790605aded212c2e3a0fbe8c015d110077ff46c07d0a6d6647a2d7e928 | commit ]]) removed the function `createForeignKeyTriggers`. This function was called in `YbATCreateSimilarForeignKey`. This is handled in this diff.
# There is a silent bug in YB master branch: in function `YbGetNext`, `desc->pkrel` is being passed to `YbAddTriggerFKReferenceIntent` as `fk_rel` argument. This change was done in [[ yugabyte@abc262c | commit ]]. This came to light on running `ALTER TABLE .. ADD FOREIGN KEY` on pg15 branch because PG introduced a equality check on `riinfo->fk_relid`  and `RelationGetRelid(trig_rel)` in [[ postgres/postgres@815b20a | commit ]]. As the bug (from YB master) and the check (from PG upstream) were couples in pg15 branch, adding FK  threw "wrong pg_constraint entry for trigger" error. This diff fixes the bug.

Test Plan: Jenkins: rebase: pg15, test regex: org.yb.pgsql.TestPgRegress.*

Reviewers: dmitry

Reviewed By: dmitry

Subscribers: dmitry, yql

Differential Revision: https://phorge.dev.yugabyte.com/D28808
Test Plan: Jenkins: rebase: pg15, test regex: org.yb.pgsql.TestPgRegress.*

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D28946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants