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

importccl: Modify row_id generation for IMPORT from CSV. #39148

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

adityamaru27
Copy link
Contributor

This change was motivated by a bug in which consecutive IMPORT INTO
queries into a table without an explicit PK, and from unique data
sources would overwrite instead of appending data.
This is because row_id generation was based on fileIndex and
rowNum which created colliding PKs across query runs.

To fix this we add the timestamp at which IMPORT INTO is run
at a 10-microsecond granularity to the rowNum.

@adityamaru27 adityamaru27 requested a review from a team July 29, 2019 15:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This change was motivated by a bug in which consecutive IMPORT INTO
queries into a table without an explicit PK, and from unique data
sources would overwrite instead of appending data.
This is because row_id generation was based on fileIndex and
rowNum which created colliding PKs across query runs.

To fix this we add the timestamp at which IMPORT INTO is run
at a 10-microsecond granularity to the rowNum.

Release note: None
@adityamaru27
Copy link
Contributor Author

bors try

craig bot pushed a commit that referenced this pull request Jul 30, 2019
@craig
Copy link
Contributor

craig bot commented Jul 30, 2019

try

Build succeeded

@adityamaru27
Copy link
Contributor Author

TFTR
bors r+

craig bot pushed a commit that referenced this pull request Jul 30, 2019
39148: importccl: Modify row_id generation for IMPORT from CSV. r=adityamaru27 a=adityamaru27

This change was motivated by a bug in which consecutive IMPORT INTO
queries into a table without an explicit PK, and from unique data
sources would overwrite instead of appending data.
This is because row_id generation was based on fileIndex and
rowNum which created colliding PKs across query runs.

To fix this we add the timestamp at which IMPORT INTO is run
at a 10-microsecond granularity to the rowNum.

39166: server: fix the advisory certificate check r=knz a=knz

Fixes #32539.

(context: `cockroach start` performs some advisory checks on the
certificates prior to starting up, to inform the user upfront if the
cert configuration is suspicious. These checks are complementary to
the actual security validation performed in the go std lib.)

Prior to this patch, the advisory cert validation was performing
checks on both the addres given to `--listen` and that given to
`--advertise` (either explicitly, or implicitly when derived from
`--listen`).

This was problematic because the listen address is really irrelevant
to security validation. What matters for node cert validation is that
the advertised address is present. So this patch removes the check on
the listen addr (recommended by @mberhault).

Additionally this patch also enhances the check on the advertise addr:
if a host name is given to `--advertise-addr` (either explicitly, or
implicitly as derived from `--listen-addr`), and the name is not
present in the cert, the validation is considered to pass if the
resolved address for the given name is present int he cert.

Release note (cli change): The advisory/informative check performed by
`cockroach start` on the validity of addresses contained in the node
certificate is relaxed to focus on the advertised node address, and to
tolerate cases when the cert contains an IP address but a hostname is
specified as advertised address.

Co-authored-by: Aditya Maru <adityamaru@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jul 30, 2019

Build succeeded

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.

3 participants