Postgres replication #7
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
charmitro
reviewed
Dec 16, 2023
| * `PATH=/path/to/pg_command` therefore we also need 5 characters for | ||
| * `PATH=` + 1 for '\0'. | ||
| */ | ||
| char *pg_bin, prefixed_command_path[261] = "PATH="; |
There was a problem hiding this comment.
Why do we need PATH= in this variable?
There was a problem hiding this comment.
Postgres needs PATH env variable to find the commands, and according to the docs it needs to be in that format.
There was a problem hiding this comment.
If i remove it I get an error from the execve command:
could not find a "pg_dump" to execute.
The process will still run though but the only way to stop the error from appearing is to have the path with PATH= prefix.
There was a problem hiding this comment.
According to the execve docs, the env variables should be of the format key=values, maybe thats why the error appears.
There was a problem hiding this comment.
No, other database driver might also use *ve due to their variables. Let's keep this consistent.
Do you think a wrapper for using execve is wise? It might help engineers use run their executable and avoid bloated code, such as
ret = execute_command(command, args, env_vars, stdout, stderr .....)There was a problem hiding this comment.
How would a wrapper work in this case? Is it to make it more contribution-friendly?
There was a problem hiding this comment.
To avoid rewriting these again and again.
Lines 206 to 230 in c324eec
There was a problem hiding this comment.
I'll do it as a follow up. Proceed with the PR as it is.
valgrind --leak-check=full --show-leak-kinds=all ./cnc -f test.yaml -v
==1745997== Memcheck, a memory error detector
==1745997== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==1745997== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==1745997== Command: ./cnc -f test.yaml -v
==1745997==
==1745997== Conditional jump or move depends on uninitialised value(s)
==1745997== at 0x484663C: strncat (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1745997== by 0x10A033: replicate (postgres.c:195)
==1745997== by 0x109581: execute_db_operations (db.c:34)
==1745997== by 0x10968A: main (main.c:45)
==1745997==
==1745997== Conditional jump or move depends on uninitialised value(s)
==1745997== at 0x4846647: strncat (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1745997== by 0x10A033: replicate (postgres.c:195)
==1745997== by 0x109581: execute_db_operations (db.c:34)
==1745997== by 0x10968A: main (main.c:45)
==1745997==
==1746000== Syscall param execve(envp[i]) points to uninitialised byte(s)
==1746000== at 0x4998917: execve (syscall-template.S:120)
==1746000== by 0x10A1D1: replicate (postgres.c:220)
==1746000== by 0x109581: execute_db_operations (db.c:34)
==1746000== by 0x10968A: main (main.c:45)
==1746000== Address 0x1fff000040 is on thread 1's stack
==1746000== in frame #1, created by replicate (postgres.c:109)
==1746000==
==1746002== Syscall param execve(envp[i]) points to uninitialised byte(s)
==1746002== at 0x4998917: execve (syscall-template.S:120)
==1746002== by 0x10A41B: replicate (postgres.c:250)
==1746002== by 0x109581: execute_db_operations (db.c:34)
==1746002== by 0x10968A: main (main.c:45)
==1746002== Address 0x1fff000040 is on thread 1's stack
==1746002== in frame #1, created by replicate (postgres.c:109)
==1746002==
test.yaml
construct
VERBOSE:connect_pg Origin-database connection: Success!
VERBOSE:connect_pg Target-database connection: Success!
VERBOSE:replicate Checking if $PG_BIN environmental variable.
VERBOSE:replicate $PG_BIN was not found, default path /usr/bin/ will be used.
VERBOSE:replicate Starting `pg_dump` process.
VERBOSE:replicate Starting `pg_restore` process.
VERBOSE:replicate Database Replication was successful!
==1745997==
==1745997== HEAP SUMMARY:
==1745997== in use at exit: 233 bytes in 5 blocks
==1745997== total heap usage: 59,760 allocs, 59,755 frees, 4,888,693 bytes allocated
==1745997==
==1745997== 8 bytes in 1 blocks are still reachable in loss record 1 of 5
==1745997== at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1745997== by 0x50192C9: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50193A1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50194E1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50179EC: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50184CF: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50049D1: gss_add_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5004FC8: gss_acquire_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5005265: gss_acquire_cred (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x48961CF: ??? (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997== by 0x488005B: PQconnectPoll (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997== by 0x48809ED: ??? (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997==
==1745997== 24 bytes in 1 blocks are still reachable in loss record 2 of 5
==1745997== at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1745997== by 0x5019273: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50193A1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50194E1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50179EC: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50184CF: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50049D1: gss_add_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5004FC8: gss_acquire_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5005265: gss_acquire_cred (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x48961CF: ??? (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997== by 0x488005B: PQconnectPoll (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997== by 0x48809ED: ??? (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997==
==1745997== 48 bytes in 1 blocks are still reachable in loss record 3 of 5
==1745997== at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1745997== by 0x51B7F0C: krb5int_setspecific (in /usr/lib/x86_64-linux-gnu/libkrb5support.so.0.1)
==1745997== by 0x50192E5: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50193A1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50194E1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50179EC: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50184CF: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50049D1: gss_add_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5004FC8: gss_acquire_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5005265: gss_acquire_cred (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x48961CF: ??? (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997== by 0x488005B: PQconnectPoll (in /usr/lib/x86_64-linux-gnu/libpq.so.5.15)
==1745997==
==1745997== 73 bytes in 1 blocks are still reachable in loss record 4 of 5
==1745997== at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1745997== by 0x49627F9: strdup (strdup.c:42)
==1745997== by 0x501938F: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50194E1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50179EC: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50184CF: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50049D1: gss_add_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5004FC8: gss_acquire_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x502D0B1: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5031D33: ??? (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x50049D1: gss_add_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997== by 0x5004FC8: gss_acquire_cred_from (in /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2.2)
==1745997==
==1745997== 80 bytes in 1 blocks are still reachable in loss record 5 of 5
==1745997== at 0x48455EF: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1745997== by 0x109489: execute_db_operations (db.c:17)
==1745997== by 0x10968A: main (main.c:45)
==1745997==
==1745997== LEAK SUMMARY:
==1745997== definitely lost: 0 bytes in 0 blocks
==1745997== indirectly lost: 0 bytes in 0 blocks
==1745997== possibly lost: 0 bytes in 0 blocks
==1745997== still reachable: 233 bytes in 5 blocks
==1745997== suppressed: 0 bytes in 0 blocks
==1745997==
==1745997== Use --track-origins=yes to see where uninitialised values come from
==1745997== For lists of detected and suppressed errors, rerun with: -s
==1745997== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0) |
Nevermind this, was testing the wrong patch. |
charmitro
requested changes
Dec 17, 2023
src/postgres.c
Outdated
| * 0 Success | ||
| * -1 Failure to connect | ||
| */ | ||
|
|
| free(pg_db_t->pg_conf); | ||
| } | ||
|
|
src/postgres.c
Outdated
Comment on lines
267
to
268
| } | ||
| void construct_pg_password(char *env_pass, const char *config_pass) |
src/postgres.c
Outdated
Comment on lines
142
to
146
| /* | ||
| * This will be used as the path for the output of `pg_dump` and the input | ||
| * of the `pg_restore`, and it will be deleted before we return from `replicate()` | ||
| */ | ||
| char dump_path[] = "/tmp/backup.dump"; |
There was a problem hiding this comment.
Two notes about this.
- We should keep it after the procedure is done for obvious reasons (debugging, examining etc.)
- Don't use
/tmp/if possible.
src/postgres.c
Outdated
| free(restore_pass); | ||
| return -2; | ||
| } else if (p_dump == 0) { | ||
| execve(strncat(command_path, "pg_dump", strlen("pg_dump") + 1), |
There was a problem hiding this comment.
Avoid calling functions inside other functions arguments.
src/postgres.c
Outdated
Comment on lines
205
to
241
| pr_info("Starting `pg_dump` process.\n"); | ||
| p_dump = fork(); | ||
| if (p_dump < 0) { | ||
| if (get_verbose()) { | ||
| pr_info("`pg_dump` process failed to start: %s. Replication process will be terminated.\n", | ||
| strerror(errno)); | ||
| } else { | ||
| printf("%s\n", strerror(errno)); | ||
| } | ||
| free(dump_pass); | ||
| free(restore_pass); | ||
| return -2; | ||
| } else if (p_dump == 0) { | ||
| execve(strncat(command_path, "pg_dump", strlen("pg_dump") + 1), | ||
| dump_args, dump_envp); | ||
| pr_info("`pg_dump` error: %s\n", strerror(errno)); | ||
| exit(-1); |
There was a problem hiding this comment.
If we dont exit there, the child process will continue running in parallel with the parent process, and we need to avoid that.
There was a problem hiding this comment.
Not if you kill the children process with its PID.
There was a problem hiding this comment.
Oh ok i see thanks!
There was a problem hiding this comment.
Im confused in a couple of things with this.
First using pipe and dup2 I can actually read the output of the command even if the command fails., so it is ok if we exit(-1) if we fail.
Second, i don't see the reason using pipes since execve will either succeed, therefore they will be no output, or it will fail, and we can see the error with pr_info("`pg_dump` error: %s\n", strerror(errno));.
There was a problem hiding this comment.
Exiting there is wrong, we need to do stuff prior to exiting our program. If you remember our design we would have logs being kept in order to save previous runs status and also send them by email.
Plus, all your pg_* commands should run with verbose on (i.e. pg_dump -v ...).
There was a problem hiding this comment.
But we don't exit the program there we just exit the child process, we can still do all the things we need in this block after:
wait(&wstatus);
// Check the status of the forked process, anything other than 0 means failure.
if (wstatus != 0) {
if (verbose) {
pr_debug(
"`pg_dump` was unsuccessful. Replication process will be terminated.\n");
} else {
printf("`pg_dump`: failed\n");
}
free(dump_pass);
free(restore_pass);
return -1;
}
There was a problem hiding this comment.
Regardless of this, we need to hold ALL STDOUT and STDERR in order to send it somewhere.
By "exiting" I mean losing the process information.
src/postgres.c
Outdated
Comment on lines
268
to
352
| void construct_pg_password(char *env_pass, const char *config_pass) | ||
| { | ||
| strncpy(env_pass, "PGPASSWORD=", strlen("PGPASSWORD=") + 1); | ||
| strncat(env_pass, config_pass, strlen(config_pass)); | ||
| } |
There was a problem hiding this comment.
Add a parameter len to the function to avoid multiple invocations of strlen here.
This commit also contains some minor memory fixes. Signed-off-by: Panagiotis Foliadis <pfoliadis@hotmail.com>
charmitro
reviewed
Dec 18, 2023
src/postgres.c
Outdated
| execve(strncat(command_path, "pg_dump", strlen("pg_dump") + 1), | ||
| dump_args, dump_envp); | ||
| pr_info("`pg_dump` error: %s\n", strerror(errno)); | ||
| exit(-1); |
There was a problem hiding this comment.
Why exit here? According to our design there is a "emailing" step you need to take into account. Exiting will not work.
charmitro
reviewed
Dec 19, 2023
src/postgres.c
Outdated
Comment on lines
208
to
210
| if (get_verbose()) { | ||
| pr_info("`pg_dump` process failed to start: %s. Replication process will be terminated.\n", | ||
| strerror(errno)); |
There was a problem hiding this comment.
pr_info already does the verbose check.
Lines 7 to 16 in a91f31b
|
I did not review it, but with a very quick look I believe the pipes of the second command ( |
I tested it and it seems to work, not sure if there is an edge case that might fail. |
|
Panagiotis Foliadis ***@***.***> writes:
I did not review it, but with a very quick look I believe the pipes of the second command (pg_restore) are not
redirecting due to them being closed at that time.
I tested it and it seems to work, not sure if there is an edge case that might fail.
I get both the verbose output of pg_dump and pg_restore.
"read_buffer" is where the output should come from, right? If that is
the case, you can test it by adding a debug word in the "printf" of the
function.
I can see it for `pg_dump` but not for `pg_restore`.
```c
printf("%s %s", __func__, buffer);
```
|
Ok I see what happened, you are right. The output of |
|
Ok I see what happened, you are right. The output of pg_restore was being printed but not through the buffer.
Shoud I use another pipe?
I suggest re-opening pipe with "pipe(pipefd)" and it'll be fine.
|
charmitro
reviewed
Dec 23, 2023
src/postgres.c
Outdated
Comment on lines
223
to
246
| p_dump = fork(); | ||
| if (p_dump == -1) { | ||
| pr_debug( | ||
| "`pg_dump` process failed to start: %s.\n Replication process will be terminated.\n", | ||
| strerror(errno)); | ||
| printf("%s\n", strerror(errno)); | ||
| free(dump_pass); | ||
| free(restore_pass); | ||
| return -2; | ||
| } else if (p_dump == 0) { | ||
| char *pg_dump_path = | ||
| strncat(command_path, "pg_dump", strlen("pg_dump") + 1); | ||
| dup2(pipefd[1], STDERR_FILENO); | ||
| close(pipefd[0]); | ||
| close(pipefd[1]); | ||
| execve(pg_dump_path, dump_args, dump_envp); | ||
| printf("Error executing `pg_dump`\n"); | ||
| pr_debug("`pg_dump` error: %s\n", strerror(errno)); | ||
| exit(-1); | ||
| } | ||
| close(pipefd[1]); | ||
| read_buffer(pipefd); | ||
|
|
||
| wait(&wstatus); |
There was a problem hiding this comment.
#define READ_END 0
#define WRITE_END 1
p_dump = fork();
if (p_dump == 0) {
char *pg_dump_path =
strncat(command_path, "pg_dump", strlen("pg_dump") + 1);
close(pipefd[READ_END]);
dup2(pipefd[1], STDOUT_FILENO);
dup2(pipefd[1], STDERR_FILENO);
close(pipefd[READ_END]);
close(pipefd[WRITE_END]);
execve(pg_dump_path, dump_args, dump_envp);
printf("Error executing `pg_dump`\n");
pr_debug("`pg_dump` error: %s\n", strerror(errno));
exit(-1);
} else {
pr_debug(
"`pg_dump` process failed to start: %s.\n Replication process will be terminated.\n",
strerror(errno));
printf("%s\n", strerror(errno));
return <return code>
}
close(pipefd[WRITE_END]);
read_buffer(pipefd);
wait(&wstatus);
|
Let's fix these sparse warnings/errors before the actual review:
$ sparse src/main.c src/config.c src/postgres.c src/db.c -Iinclude/ -I/usr/include/postgresql/ -Iinclude/db/
src/main.c:15:45: warning: Using plain integer as NULL pointer
src/main.c:16:35: warning: Using plain integer as NULL pointer
src/postgres.c:25:18: warning: non-ANSI function declaration of
function 'construct_pg'
src/postgres.c:25:5: warning: symbol 'construct_pg' was not
declared. Should it be static?
src/postgres.c:57:54: warning: Using plain integer as NULL pointer
src/postgres.c:166:17: warning: Using plain integer as NULL pointer
src/postgres.c:180:17: warning: Using plain integer as NULL pointer
src/postgres.c:215:27: warning: Using plain integer as NULL pointer
src/postgres.c:254:27: warning: Using plain integer as NULL pointer
Install sparse via (it is already installed on our development machine):
$ sudo apt install sparse
|
panosfol
force-pushed
the
pg-connect
branch
2 times, most recently
from
ccea3d3
to
1c00955
Compare
charmitro
reviewed
Jan 1, 2024
src/postgres.c
Outdated
Comment on lines
124
to
126
| char *pg_pass = | ||
| (char *)calloc(prefixed_command_pass_size, | ||
| prefixed_command_pass_size * sizeof(char)); |
There was a problem hiding this comment.
Suggested change
| char *pg_pass = | |
| (char *)calloc(prefixed_command_pass_size, | |
| prefixed_command_pass_size * sizeof(char)); | |
| char *pg_pass = calloc(prefixed_command_pass_size, sizeof(char)); |
This should be enough. Casting and multiplying size with sizeof(char) is not needed also.
There was a problem hiding this comment.
I run valgrind after making these changes, and I noticed that if i dont cast it as char * I get one more heap alloc, and one more free. Not sure if we care about this but I found it interesting. Should I cast it after all?
There was a problem hiding this comment.
Ok that was a mistake from my side. If i DONT cast i get these results so I will for sure not cast.
There was a problem hiding this comment.
Ok I can't seem to figure it out, I still get one random alloc more irregardless if i cast as char * or not.
There was a problem hiding this comment.
Not sure what you are referring to. This is valgrind outputs (diffed) before and after.
--- #<buffer val2>
+++ #<buffer val>
@@ -1,9 +1,9 @@
valgrind --leak-check=full ./cnc -f test.yaml
-==610146== Memcheck, a memory error detector
-==610146== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
-==610146== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
-==610146== Command: ./cnc -f test.yaml
-==610146==
+==610191== Memcheck, a memory error detector
+==610191== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
+==610191== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
+==610191== Command: ./cnc -f test.yaml
+==610191==
test.yaml
pg_dump: last built-in OID is 16383
pg_dump: reading extensions
@@ -57,19 +57,19 @@
pg_restore: connecting to database for restore
pg_restore: implied data-only restore
Database Replication was successful!
-==610146==
-==610146== HEAP SUMMARY:
-==610146== in use at exit: 153 bytes in 4 blocks
-==610146== total heap usage: 59,762 allocs, 59,758 frees, 4,888,620 bytes allocated
-==610146==
-==610146== LEAK SUMMARY:
-==610146== definitely lost: 0 bytes in 0 blocks
-==610146== indirectly lost: 0 bytes in 0 blocks
-==610146== possibly lost: 0 bytes in 0 blocks
-==610146== still reachable: 153 bytes in 4 blocks
-==610146== suppressed: 0 bytes in 0 blocks
-==610146== Reachable blocks (those to which a pointer was found) are not shown.
-==610146== To see them, rerun with: --leak-check=full --show-leak-kinds=all
-==610146==
-==610146== For lists of detected and suppressed errors, rerun with: -s
-==610146== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
+==610191==
+==610191== HEAP SUMMARY:
+==610191== in use at exit: 153 bytes in 4 blocks
+==610191== total heap usage: 59,762 allocs, 59,758 frees, 4,888,240 bytes allocated
+==610191==
+==610191== LEAK SUMMARY:
+==610191== definitely lost: 0 bytes in 0 blocks
+==610191== indirectly lost: 0 bytes in 0 blocks
+==610191== possibly lost: 0 bytes in 0 blocks
+==610191== still reachable: 153 bytes in 4 blocks
+==610191== suppressed: 0 bytes in 0 blocks
+==610191== Reachable blocks (those to which a pointer was found) are not shown.
+==610191== To see them, rerun with: --leak-check=full --show-leak-kinds=all
+==610191==
+==610191== For lists of detected and suppressed errors, rerun with: -s
+==610191== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)There was a problem hiding this comment.
I run valgrind 2 times consecutively without changing anything. First output:
==610288==
==610288== HEAP SUMMARY:
==610288== in use at exit: 153 bytes in 4 blocks
==610288== total heap usage: 59,762 allocs, 59,758 frees, 4,885,168 bytes allocated
==610288==
==610288== LEAK SUMMARY:
==610288== definitely lost: 0 bytes in 0 blocks
==610288== indirectly lost: 0 bytes in 0 blocks
==610288== possibly lost: 0 bytes in 0 blocks
==610288== still reachable: 153 bytes in 4 blocks
==610288== suppressed: 0 bytes in 0 blocks
==610288== Reachable blocks (those to which a pointer was found) are not shown.
==610288== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==610288==
==610288== For lists of detected and suppressed errors, rerun with: -s
==610288== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Second output:
==610297==
==610297== HEAP SUMMARY:
==610297== in use at exit: 153 bytes in 4 blocks
==610297== total heap usage: 59,763 allocs, 59,759 frees, 4,917,984 bytes allocated
==610297==
==610297== LEAK SUMMARY:
==610297== definitely lost: 0 bytes in 0 blocks
==610297== indirectly lost: 0 bytes in 0 blocks
==610297== possibly lost: 0 bytes in 0 blocks
==610297== still reachable: 153 bytes in 4 blocks
==610297== suppressed: 0 bytes in 0 blocks
==610297== Reachable blocks (those to which a pointer was found) are not shown.
==610297== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==610297==
==610297== For lists of detected and suppressed errors, rerun with: -s
==610297== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
There was a problem hiding this comment.
We don't really care about these allocations , we should care about total byte allocations, which is much lower with the change.
There was a problem hiding this comment.
I read wrong probably. Can you diff your code and paste it?
There was a problem hiding this comment.
Yeah I agree but why do I get 2 different outcomes without changing anything? Is it an OS thing?
There was a problem hiding this comment.
I don't know which gcc and valgrind versions you use. GCC can have differences in the assembly generations between versions.
charmitro
requested changes
Jan 1, 2024
src/postgres.c
Outdated
Comment on lines
115
to
116
| int command_pass_size = strlen(pg_db_t->pg_conf->origin_password) + 1; | ||
| int prefixed_command_pass_size = PG_PASS_PREFIX + command_pass_size; |
There was a problem hiding this comment.
I'd argue that command_pass is not the correct variable name for these. Maybe pg_pass_size or pass_env_var_size or whatever?
src/postgres.c
Outdated
| int command_pass_size = strlen(pg_db_t->pg_conf->origin_password) + 1; | ||
| int prefixed_command_pass_size = PG_PASS_PREFIX + command_pass_size; | ||
| int pg_command_size = strlen(PG_DUMP_COMMAND); | ||
| int prefix_pass_size = strlen("PGPASSWORD="); |
There was a problem hiding this comment.
Do we need this variable? Isn't PG_PASS_PREFIX the same?
src/postgres.c
Outdated
Comment on lines
299
to
307
| /* | ||
| * execve_binary | ||
| * | ||
| * 0 Success | ||
| * -1 Failure of`pg_dump` or `pg_restore` | ||
| * -2 Failure of creation of fork() or pipe() | ||
| */ |
There was a problem hiding this comment.
Maybe some documentation here?
src/postgres.c
Outdated
Comment on lines
100
to
116
| /* | ||
| * replicate | ||
| * | ||
| * 0 Success | ||
| * -1 Failure of`pg_dump` or `pg_restore` | ||
| * -2 Failure of creation of fork() or pipe() | ||
| */ |
There was a problem hiding this comment.
Maybe some documentation here?
The main logic of `replicate` is achieved by using `fork` to create two new processes, one for `pg_dump` and one for `pg_restore`. The commit also contains utilization of `log.h` library and some fixes for better error handling. Signed-off-by: Panagiotis Foliadis <pfoliadis@hotmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains the following:
pg_connect: Connection to the origin and the target database.replicate: Utilize thepg_dumpandpg_restorecommands to replicate the database.log.hlibrary.