-
Notifications
You must be signed in to change notification settings - Fork 161
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
feat: Adds 'skip-empty-xacts' support #248
base: master
Are you sure you want to change the base?
Conversation
@eulerto Please have a look and let me know what changes are required |
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.
Initial review.
-- predictability | ||
SET synchronous_commit = on; | ||
|
||
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'wal2json'); |
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.
Do these tests exercise the proposal? Why don't you use simple statements (for example, using filter-tables
or add-tables
option)? You should include tests for both formats (see other test files).
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.
ok
wal2json.c
Outdated
{ | ||
JsonDecodingData *data = ctx->output_plugin_private; | ||
|
||
data->nr_changes = 0; | ||
|
||
/* Transaction starts */ | ||
OutputPluginPrepareWrite(ctx, true); | ||
OutputPluginPrepareWrite(ctx, last_write); |
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.
Did you test this modification with the write-in-chunks
option?
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.
no i have not
i will benchmark this change with write-in-chunks
option
wal2json.c
Outdated
@@ -170,7 +186,7 @@ static void pg_decode_truncate_v1(LogicalDecodingContext *ctx, | |||
|
|||
/* version 2 */ | |||
static void pg_decode_begin_txn_v2(LogicalDecodingContext *ctx, | |||
ReorderBufferTXN *txn); | |||
ReorderBufferTXN *txn, bool last_write); |
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.
There is an additional space here.
* has requested to skip the empty transactions we can skip the empty streams | ||
* even though the transaction has written some changes. | ||
*/ | ||
typedef struct |
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.
Why do you need this new struct? Can't you use data->nr_changes
?
@eulerto can you please go through the comments I posted? |
I don't follow. You asked a question about |
thanks I will go through that. I have addressed all of your comments and made changes accordingly. I am stuck on |
If it is changing a lot, don't run it. Instead, fix some superfluous changes (such as whitespace and new line additions). I can clean it up later, if required. |
yeah sure thanks |
fixes #106