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

use unlogged table for cataloger diff results #685

Merged
merged 4 commits into from
Oct 1, 2020
Merged

Conversation

nopcoder
Copy link
Contributor

No description provided.

@nopcoder nopcoder added the area/cataloger Improvements or additions to the cataloger label Sep 30, 2020
@nopcoder nopcoder requested review from ozkatz and tzahij September 30, 2020 16:21
@nopcoder nopcoder self-assigned this Sep 30, 2020
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks cool, except for xid. I read the docs, xid relies heavily on nonrandomness for uniqueness, and the nonrandom hostid component used by this particular implementation seems already to have known failure modes.

@nopcoder nopcoder requested a review from arielshaqed October 1, 2020 07:19
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unquoted UUID also fails. nanoid or prepend a string, see below.

// contextWithDiffResultsDispose generate diff results id used for temporary table name
func contextWithDiffResultsDispose(ctx context.Context, tx sq.Execer) (context.Context, context.CancelFunc) {
id := xid.New().String()
// withDiffResultsContext generate diff results id used for temporary table name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// withDiffResultsContext generate diff results id used for temporary table name
// withDiffResultsContext generates diff results id used for temporary table name

id := xid.New().String()
// withDiffResultsContext generate diff results id used for temporary table name
func (c *cataloger) withDiffResultsContext(ctx context.Context) (context.Context, context.CancelFunc) {
id := strings.ReplaceAll(uuid.New().String(), "-", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this produces a valid table name only 37.5% of the time, when the first hex digit is a-f. You might prepend "diff_table_" to this string.
Or just use nanoid, which is designed to produce useful unescaped names :-/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it already gets a prefix.

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is of course cool.

See golang func comment grammar fix to use the approved golang present tense.

id := xid.New().String()
// withDiffResultsContext generate diff results id used for temporary table name
func (c *cataloger) withDiffResultsContext(ctx context.Context) (context.Context, context.CancelFunc) {
id := strings.ReplaceAll(uuid.New().String(), "-", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it already gets a prefix.

@nopcoder nopcoder merged commit 0f4fb7f into master Oct 1, 2020
@nopcoder nopcoder deleted the chore/temp-table branch February 4, 2021 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cataloger Improvements or additions to the cataloger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants