fix(schema_statements): add unique_rowid to pk_and_sequence_for

[BuonOmo]

Allow to get the primary key in the default case.

This fixes two of the four failing test_pk_and_sequence_for*. The two still failing are:

test_pk_and_sequence_for_returns_nil_if_no_pk

      def test_pk_and_sequence_for_returns_nil_if_no_pk
        with_example_table "id integer" do
          assert_nil @connection.pk_and_sequence_for("ex") # => `["rowid", nil]`
        end
      end

the example table creation query:

CREATE TABLE ex(id integer)

The second query of #pk_and_sequence_for returns ["rowid", "public", nil]. And the SHOW CREATE TABLE ex confirms it:

CREATE TABLE public.ex (
	id INT8 NULL,
	rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
	CONSTRAINT ex_pkey PRIMARY KEY (rowid ASC)
)

test_pk_and_sequence_for_with_collision_pg_class_oid

create table ex(id serial primary key);
create table ex2(id serial primary key);
DELETE FROM pg_depend WHERE objid = 'ex_id_seq'::regclass AND refobjid = 'ex'::regclass AND deptype = 'a';

Gives the error:

user root does not have DELETE privilege on relation pg_depend

The bug is not related to the code, but just to the tests. @rafiss could we give a user this privilege ? I didn't see it skimming the doc... (show grants gives f for is_grantable on pg_depend...):

grant DELETE on pg_depend to root;
--> ERROR: invalid privilege type DELETE for virtual_table

---

On meta-testing. This took me longer than expected. I'm a bit unsure about the return on investment for simple tests, as manipulating AST can feel a bit tricky. I still pursued it because it seems that the gems (parser and unparser) are quite stable, and if we could use this not only in tests, that would be tremendous. We would need more guaranties though. And we'd need to verify the performances..

I was thinking about checking the hash of the method to be changed against a saved hash (which would be stored in an argument). If there is a change we warn. So:

CopyCat.copy_methods(..., hash: "somehash") do ... end
# => WARN: the method XXX was updated, here's the new changed method: ...
#      If it is ok change the hash to "somenewhash"

I think this is the way to go, but there are definitely some tweaks to make it easier.

[BuonOmo]

@rafiss does this query make sense for CRDB ? All the tests I'm working with until now are giving a void result...

[rafiss]

yes, this looks correct

if you are getting no results, it's probably because CRDB defaults to not using sequences for SERIAL types. this can be changed with the serial_normalization setting: https://www.cockroachlabs.com/docs/stable/serial#modes-of-operation

[BuonOmo]

Somehow the test test_pk_and_sequence_for_returns_nil_if_no_pk works locally and not on CI...

[rafiss]

Gives the error:

user root does not have DELETE privilege on relation pg_depend

The bug is not related to the code, but just to the tests. @rafiss could we give a user this privilege ? I didn't see it skimming the doc... (show grants gives f for is_grantable on pg_depend...):

Unfortunately, pg_depend is view-only in CRDB, unlike PG. In CRDB there is no way to "artificially break" the dependency that the sequence has on the table.

[rafiss]

yes, this looks correct

if you are getting no results, it's probably because CRDB defaults to not using sequences for SERIAL types. this can be changed with the serial_normalization setting: https://www.cockroachlabs.com/docs/stable/serial#modes-of-operation

[BuonOmo]

About test_pk_and_sequence_for_returns_nil_if_no_pk. The table creation returns a line:

rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid()

This line is the one that is failing us...

[rafiss]

it could be useful to link to cockroachdb/cockroach#126397 in the TODO, so we can easily see when the new column is in a CRDB release.

[BuonOmo]

It should be good for merging now! I think I'll work on the transaction related issues next, and then we'll almost be done with those red tests.

[rafiss]

lgtm!