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

Created wrappers around Internal IDs for consistency and added an ID dispatch registry #1107

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Hydrocharged
Copy link
Collaborator

@Hydrocharged Hydrocharged commented Jan 8, 2025

Currently, an Internal ID requires one to know the exact format for constructing an ID of a specific type. For example, Internal table IDs have the following format:

[ Schema_Name, Table_Name ]

Not only would you need to know that this is the format, but when retrieving values, you'd have to make sure that you accessed index 0 for the schema, and index 1 for the table. This opens up a potential for hard-to-find bugs. Wrappers help with this. Internally, the wrappers will call index 0 or 1, but one now just calls SchemaName() or TableName(). In addition, they don't need to know the format, calling NewInternalTable gives you the parameters that you need.

Additionally, this adds a registry where schema elements may register callbacks to respond to ID changes. For example, rather than having a table column explicitly handle all locations that depend on the column, it instead broadcasts that the column has changed, and all locations that have registered for that broadcast will modify themselves. This helps with scaling, as Postgres is far more inter-dependent than MySQL/Dolt, and reduces the knowledge burden required by developers.

Currently it's only used by sequences in this PR, and that's primarily due to most actions being handed off to GMS and Doltgres having no record of them. For example, deleting a column is completely handled by GMS, but Doltgres needs to broadcast that the column was deleted. For now, the idea is to wrap all such nodes in a Doltgres node at the end of the analyzer step (in ReplaceNode), but I'm open to another idea since it essentially means we're making a wrapper for the majority of GMS nodes. That's a separate issue from this PR though.

@Hydrocharged Hydrocharged requested a review from zachmu January 8, 2025 14:39
@Hydrocharged Hydrocharged force-pushed the daylon/internal-renaming branch from 231804f to 738599c Compare January 8, 2025 14:44
Copy link
Contributor

github-actions bot commented Jan 8, 2025

Main PR
covering_index_scan_postgres 365.62/s 367.03/s +0.3%
index_join_postgres 151.95/s 150.59/s -0.9%
index_join_scan_postgres 181.85/s 182.23/s +0.2%
index_scan_postgres 12.40/s 12.15/s -2.1%
oltp_point_select 2825.83/s 2814.83/s -0.4%
oltp_read_only 1899.54/s 1913.93/s +0.7%
select_random_points 113.06/s 110.40/s -2.4%
select_random_ranges 133.05/s 134.78/s +1.3%
table_scan_postgres 11.77/s 11.82/s +0.4%
types_table_scan_postgres 5.51/s 5.57/s +1.0%

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Main PR
Total 42090 42090
Successful 15383 15442
Failures 26707 26648
Partial Successes1 5237 5236
Main PR
Successful 36.5479% 36.6880%
Failures 63.4521% 63.3120%

${\color{lightgreen}Progressions (15)}$

alter_table

QUERY: create table atacc1 (id serial primary key, value int check (value < 10));
QUERY: alter table atacc1 drop column value;
QUERY: alter table atacc1 add column value int check (value < 10);

fast_default

QUERY: ALTER TABLE t ADD COLUMN y int;
QUERY: ALTER TABLE t ADD COLUMN x int;
QUERY: ALTER TABLE t ADD COLUMN x int;
QUERY: ALTER TABLE t ADD COLUMN y int;
QUERY: INSERT INTO t (a,b,c) VALUES (1,2,NULL);
QUERY: INSERT INTO t (a,b,c) VALUES (1,2,NULL);
QUERY: ALTER TABLE t ADD COLUMN y int;
QUERY: INSERT INTO t (a,b,c) VALUES (1,2,NULL);
QUERY: ALTER TABLE t ADD COLUMN x int;
QUERY: INSERT INTO t (a,b,c) VALUES (1,2,NULL);
QUERY: ALTER TABLE t ADD COLUMN x int;
QUERY: ALTER TABLE t ADD COLUMN y int;

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@Hydrocharged Hydrocharged force-pushed the daylon/internal-renaming branch from 738599c to 90cd854 Compare January 10, 2025 14:20
@Hydrocharged Hydrocharged changed the title Created wrappers around Internal IDs for consistency Created wrappers around Internal IDs for consistency and added an ID dispatch registry Jan 10, 2025
@Hydrocharged Hydrocharged force-pushed the daylon/internal-renaming branch from 90cd854 to 6cef540 Compare January 10, 2025 14:33
@Hydrocharged
Copy link
Collaborator Author

Tests are failing from changes made in the merged main (forgot to run tests again after merging main). Nothing core will change, just need to update them, so PR is still good to review.

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.

1 participant