-
Notifications
You must be signed in to change notification settings - Fork 123
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 COPY statement for flush #1508
Conversation
await database.retry(async () => { | ||
await database.transaction(async (client, tx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this an oversight from the tx PR (run setup functions in tx) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really an oversight, it just wasn't necessary
schema: schemaBuild.schema, | ||
}); | ||
const result = await fn(client, tx); | ||
await client.query("COMMIT"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that if we'd rather not use the pool-level SET
trick, you can set the current transaction to be asynchronous.
This parameter can be changed at any time; the behavior for any one transaction is determined by the setting in effect when it commits. It is therefore possible, and useful, to have some transactions commit synchronously and others asynchronously. For example, to make a single multistatement transaction commit asynchronously when the default is the opposite, issue SET LOCAL synchronous_commit TO OFF within the transaction.
packages/core/src/database/index.ts
Outdated
await client.query("COMMIT"); | ||
return result; | ||
} catch (error) { | ||
await client?.query("ROLLBACK"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is client nullable?
|
||
common.logger.debug({ | ||
service: "database", | ||
msg: `Inserted ${insertValues.length} '${getTableName(table)}' rows`, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this unintentionally reverts the logs I changed
|
||
const text = getCopyText( | ||
table, | ||
insertValues.map(({ row }) => row), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we have to be careful last time about memory here, to make sure the insertValues are garbage-collectable as we build the text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, yes. That was when we were handling a cache that was too large and flushing buffer entries to the database at the same step. Now because the buffer entries get flushed in smaller and more frequent chunks it isn't necessary.
export const getCopyText = ( | ||
table: Table, | ||
rows: { [key: string]: unknown }[], | ||
) => { | ||
const columns = Object.entries(getTableColumns(table)); | ||
const results: string[] = []; | ||
for (const row of rows) { | ||
const values: string[] = []; | ||
for (const [columnName, column] of columns) { | ||
let value = row[columnName]; | ||
if (value === null || value === undefined) { | ||
values.push("\\N"); | ||
} else { | ||
if (column.mapToDriverValue !== undefined) { | ||
value = column.mapToDriverValue(value); | ||
} | ||
if (value === null || value === undefined) { | ||
values.push("\\N"); | ||
} else { | ||
values.push(String(value).replace(/\\/g, "\\\\")); | ||
} | ||
} | ||
} | ||
results.push(values.join("\t")); | ||
} | ||
return results.join("\n"); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@typedarray Is this what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup LGTM
Closes #1469