sql/opt: make TG_ARGV use 0-based indexing for PostgreSQL compatibility

[rafiss]

Previously, TG_ARGV used 1-based indexing (standard SQL array behavior), which was incompatible with PostgreSQL's 0-based indexing for TG_ARGV. A session setting allow_create_trigger_function_with_argv_references was required to use TG_ARGV at all, since it was blocked by default due to this incompatibility.

This change makes TG_ARGV use 0-based indexing to match PostgreSQL behavior. To implement this, we add a zeroIndexed field to the DArray struct that controls the FirstIndex() return value. When constructing TG_ARGV for trigger functions, we now call SetZeroIndexed() to mark the array as 0-indexed.

The allow_create_trigger_function_with_argv_references session setting is no longer needed and has been converted to a backwards-compatibility no-op variable.

Fixes: #135311

Release note (backward-incompatible change): The TG_ARGV trigger function parameter now uses 0-based indexing to match PostgreSQL behavior. Previously, TG_ARGV[1] returned the first argument; now TG_ARGV[0] returns the first argument and TG_ARGV[1] returns the second argument. Additionally, usage of TG_ARGV no longer requires setting the allow_create_trigger_function_with_argv_references session variable.

[cockroach-teamcity]

This change is 

[DrewKimball]

Nice job Rafi!

@DrewKimball reviewed 11 files and all commit messages, and made 3 comments.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mw5h and @rafiss).

---

pkg/sql/sem/tree/datum.go line 4902 at r1 (raw file):

// SetZeroIndexed marks this array as 0-indexed. This is used for TG_ARGV
// to match PostgreSQL's 0-indexed behavior.
func (d *DArray) SetZeroIndexed() {

Wow, nice work! I didn't realize this would require so few changes.

---

pkg/ccl/logictestccl/testdata/logic_test/triggers line 4805 at r1 (raw file):

statement ok
CREATE FUNCTION g() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$

It would be a good idea to add some more tests:

• DECLARE a TEXT[] variable, assign TG_ARGV to it, then try the zero-indexed read.
• The above, but assign into the variable using SELECT ... INTO .
• DECLARE a function that takes a TEXT[] argument and tries a zero-indexed read on it.

[rafiss]

@rafiss made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @mw5h).

---

pkg/ccl/logictestccl/testdata/logic_test/triggers line 4805 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

It would be a good idea to add some more tests:

• DECLARE a TEXT[] variable, assign TG_ARGV to it, then try the zero-indexed read.
• The above, but assign into the variable using SELECT ... INTO .
• DECLARE a function that takes a TEXT[] argument and tries a zero-indexed read on it.

i added the first and third test cases and verified that it matches postgres behavior. are those what you had in mind?

for the second test case you asked for, there is a separate problem that seems to prevent SELECT ... INTO from working on TG_* variables. i got this error even when trying with TG_NAME:

statement ok
CREATE TABLE xy (x INT, y INT);

statement ok
CREATE FUNCTION g() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
  DECLARE
    args TEXT;
  BEGIN
    SELECT TG_NAME INTO args;
    RAISE NOTICE 'args: %', args;
    RETURN NEW;
  END
$$;

statement ok
CREATE TRIGGER foo BEFORE INSERT ON xy FOR EACH ROW EXECUTE FUNCTION g('into_one', 'into_two');

statement ok
INSERT INTO xy VALUES (101, 201);

[16:25:59] INSERT INTO xy VALUES (101, 201);;
[16:25:59] 	-- OK;
[16:25:59] --- done: /Users/rafiss/.cache/bazel/_bazel_rafiss/bases/cf10ff/1/sandbox/darwin-sandbox/2452/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/ccl/logictestccl/tests/local/local_test_/local_test.runfiles/com_github_cockroachdb_cockroach/pkg/ccl/logictestccl/testdata/logic_test/_tmp with config local: 3 tests, 0 failures

    logic.go:4653:
        /Users/rafiss/.cache/bazel/_bazel_rafiss/bases/cf10ff/1/sandbox/darwin-sandbox/2452/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/ccl/logictestccl/tests/local/local_test_/local_test.runfiles/com_github_cockroachdb_cockroach/pkg/ccl/logictestccl/testdata/logic_test/_tmp:21: error while processing
    logic.go:4653:
        /Users/rafiss/.cache/bazel/_bazel_rafiss/bases/cf10ff/1/sandbox/darwin-sandbox/2452/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/ccl/logictestccl/tests/local/local_test_/local_test.runfiles/com_github_cockroachdb_cockroach/pkg/ccl/logictestccl/testdata/logic_test/_tmp:21:
        expected success, but found
        (XX000) internal error: top-level relational expression cannot have outer columns: (38)
        optimizer.go:277: in Optimize()
        DETAIL: stack trace:
        pkg/sql/opt/xform/optimizer.go:277: Optimize()
        pkg/sql/opt/exec/execbuilder/scalar.go:1288: func2()
        pkg/sql/routine.go:329: startInternal()
        pkg/sql/routine.go:279: Start()
        pkg/sql/routine.go:179: EvalRoutineExpr()
        pkg/sql/sem/eval/expr.go:648: EvalRoutineExpr()
        bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/sem/tree/eval_expr_generated.go:361: Eval()
        pkg/sql/sem/eval/expr.go:22: Expr()
        pkg/sql/execinfra/execexpr/expr.go:310: eval()
        pkg/sql/execinfra/execexpr/expr.go:163: EvalExpr()
        pkg/sql/execinfra/processorsbase.go:294: ProcessRow()
        pkg/sql/execinfra/processorsbase.go:744: ProcessRowHelper()
        pkg/sql/rowexec/noop.go:96: Next()
        pkg/sql/colexec/columnarizer.go:244: Next()
        pkg/sql/colexec/invariants_checker.go:99: Next()
        pkg/sql/colexec/materializer.go:248: next()
        pkg/sql/colexec/materializer.go:276: nextAdapter()
        pkg/sql/colexecerror/error.go:162: CatchVectorizedRuntimeError()
        pkg/sql/colexec/materializer.go:282: Next()
        pkg/sql/colflow/flow_coordinator.go:119: next()
        pkg/sql/colflow/flow_coordinator.go:136: nextAdapter()
        pkg/sql/colexecerror/error.go:162: CatchVectorizedRuntimeError()
        pkg/sql/colflow/flow_coordinator.go:141: Next()
        pkg/sql/execinfra/base.go:193: Run()
        pkg/sql/execinfra/processorsbase.go:766: Run()
        pkg/sql/flowinfra/flow.go:574: Run()
        pkg/sql/colflow/vectorized_flow.go:302: Run()
        pkg/sql/distsql_running.go:1087: Run()
        pkg/sql/distsql_running.go:2353: PlanAndRun()
        pkg/sql/apply_join.go:378: func2()
        pkg/sql/apply_join.go:381: runPlanInsidePlan()
        pkg/sql/routine.go:413: func1()

@DrewKimball is that already tracked in a different issue?

[mw5h]

@mw5h reviewed 11 files and all commit messages, and made 1 comment.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @rafiss).

[DrewKimball]

@DrewKimball reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @rafiss).

---

pkg/ccl/logictestccl/testdata/logic_test/triggers line 4805 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i added the first and third test cases and verified that it matches postgres behavior. are those what you had in mind?

for the second test case you asked for, there is a separate problem that seems to prevent SELECT ... INTO from working on TG_* variables. i got this error even when trying with TG_NAME:

statement ok
CREATE TABLE xy (x INT, y INT);

# Test assigning TG_ARGV to a TEXT[] variable using SELECT ... INTO.
# TODO(#???): This should work like PostgreSQL, but currently the INSERT causes an
# internal error.
statement ok
CREATE FUNCTION g() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
  DECLARE
    args TEXT;
  BEGIN
    SELECT TG_NAME INTO args;
    RAISE NOTICE 'args: %', args;
    RETURN NEW;
  END
$$;

statement ok
CREATE TRIGGER foo BEFORE INSERT ON xy FOR EACH ROW EXECUTE FUNCTION g('into_one', 'into_two');

statement ok
INSERT INTO xy VALUES (101, 201);

[16:25:59] INSERT INTO xy VALUES (101, 201);;
[16:25:59] 	-- OK;
[16:25:59] --- done: /Users/rafiss/.cache/bazel/_bazel_rafiss/bases/cf10ff/1/sandbox/darwin-sandbox/2452/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/ccl/logictestccl/tests/local/local_test_/local_test.runfiles/com_github_cockroachdb_cockroach/pkg/ccl/logictestccl/testdata/logic_test/_tmp with config local: 3 tests, 0 failures

    logic.go:4653:
        /Users/rafiss/.cache/bazel/_bazel_rafiss/bases/cf10ff/1/sandbox/darwin-sandbox/2452/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/ccl/logictestccl/tests/local/local_test_/local_test.runfiles/com_github_cockroachdb_cockroach/pkg/ccl/logictestccl/testdata/logic_test/_tmp:21: error while processing
    logic.go:4653:
        /Users/rafiss/.cache/bazel/_bazel_rafiss/bases/cf10ff/1/sandbox/darwin-sandbox/2452/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/ccl/logictestccl/tests/local/local_test_/local_test.runfiles/com_github_cockroachdb_cockroach/pkg/ccl/logictestccl/testdata/logic_test/_tmp:21:
        expected success, but found
        (XX000) internal error: top-level relational expression cannot have outer columns: (38)
        optimizer.go:277: in Optimize()
        DETAIL: stack trace:
        pkg/sql/opt/xform/optimizer.go:277: Optimize()
        pkg/sql/opt/exec/execbuilder/scalar.go:1288: func2()
        pkg/sql/routine.go:329: startInternal()
        pkg/sql/routine.go:279: Start()
        pkg/sql/routine.go:179: EvalRoutineExpr()
        pkg/sql/sem/eval/expr.go:648: EvalRoutineExpr()
        bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/sem/tree/eval_expr_generated.go:361: Eval()
        pkg/sql/sem/eval/expr.go:22: Expr()
        pkg/sql/execinfra/execexpr/expr.go:310: eval()
        pkg/sql/execinfra/execexpr/expr.go:163: EvalExpr()
        pkg/sql/execinfra/processorsbase.go:294: ProcessRow()
        pkg/sql/execinfra/processorsbase.go:744: ProcessRowHelper()
        pkg/sql/rowexec/noop.go:96: Next()
        pkg/sql/colexec/columnarizer.go:244: Next()
        pkg/sql/colexec/invariants_checker.go:99: Next()
        pkg/sql/colexec/materializer.go:248: next()
        pkg/sql/colexec/materializer.go:276: nextAdapter()
        pkg/sql/colexecerror/error.go:162: CatchVectorizedRuntimeError()
        pkg/sql/colexec/materializer.go:282: Next()
        pkg/sql/colflow/flow_coordinator.go:119: next()
        pkg/sql/colflow/flow_coordinator.go:136: nextAdapter()
        pkg/sql/colexecerror/error.go:162: CatchVectorizedRuntimeError()
        pkg/sql/colflow/flow_coordinator.go:141: Next()
        pkg/sql/execinfra/base.go:193: Run()
        pkg/sql/execinfra/processorsbase.go:766: Run()
        pkg/sql/flowinfra/flow.go:574: Run()
        pkg/sql/colflow/vectorized_flow.go:302: Run()
        pkg/sql/distsql_running.go:1087: Run()
        pkg/sql/distsql_running.go:2353: PlanAndRun()
        pkg/sql/apply_join.go:378: func2()
        pkg/sql/apply_join.go:381: runPlanInsidePlan()
        pkg/sql/routine.go:413: func1()

@DrewKimball is that already tracked in a different issue?

Thanks for adding the tests. This internal error is new to me, weird...

[rafiss]

TFTR!

bors r+

@rafiss made 2 comments.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball).

---

pkg/ccl/logictestccl/testdata/logic_test/triggers line 4805 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Thanks for adding the tests. This internal error is new to me, weird...

i filed #162289 to track the internal error.

[craig]

Build succeeded:

• acceptance
• check_generated_code
• lint
• linux_amd64_build
• local_roachtest
• local_roachtest_fips
• examples_orms
• docker_image_amd64
• unit_tests
• linux_amd64_fips_build