NotificationsYou must be signed in to change notification settings
Fork91
Star761

Small fixes#546

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

funny-falcon merged 5 commits intomasterfromREL_2_5-some-fixes

Oct 13, 2022

Merged

Small fixes#546

funny-falcon merged 5 commits intomasterfromREL_2_5-some-fixes

Oct 13, 2022

Conversation

Copy link

Contributor

funny-falcon commentedOct 11, 2022

No description provided.

funny-falcon changed the base branch frommaster toREL_2_5

October 11, 2022 06:38

funny-falcon requested review fromgsmolk andkulaginm

October 11, 2022 06:40

kulaginm reviewed

Oct 11, 2022

View reviewed changes

src/pg_probackup.c

		elog(ERROR,"Version mismatch, pg_probackup binary with version '%s' "
		"is launched as an agent for pg_probackup binary with version '%s'",
		PROGRAM_VERSION,argv[2]);
		remote_agent= true;

Copy link

Member

Choose a reason for hiding this comment

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

Я думаю тогда надо дочистить и упоминание условия в config_get_opt() (потому что в случае агента эта функция вроде и не вызывается).

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

done

kulaginm requested changes

Oct 11, 2022

View reviewed changes

src/utils/file.c Outdated

		break;
		caseFIO_GET_CRC32:
		/* calculate crc32 for a file */
		if (hdr.arg==1)

Copy link

Member

Choose a reason for hiding this comment

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

А это условие разве остаётся корректным после введения битовых флагов в arg?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

! Спасибо!

src/data.cShow resolvedHide resolved

src/archive.c Outdated

		pg_crc32crc32_src;
		pg_crc32crc32_dst;

		/* TODO: what if one of them goes missing? */

Copy link

Member

Choose a reason for hiding this comment

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

А что с этим комментарием? Он актуален?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Хороший вопрос: могут ли у нас под ногами удалить файл?
Думаю, пока можно "забить" и убрать комментарий.
В целом, даже если мы хотим передавать missing_ok=true , нужно различать "не существовавший" файл и файл с crc==0. Так что, придётся переделывать API.

Copy link

Member

kulaginm commentedOct 11, 2022•
edited
Loading

По поводу введения set_forkname:
Первое, за что зацепился глаз -- это то, что функция глобальная, а мне кажется, что не надо выносить это за пределы src/dir.c.
Предлагаю рассматривать pgFileInit() как некоторый "конструктор" "объектов" pgFile и внести логику/вызов заполнения forkName внутрь этого "конструктора".
Тут остаётся вопрос с обработкой external_dir_num, вижу два варианта:
или заполнять поле forkName в любом случае (так как оно зависит от имени), а обработку external_dir_num оставить на внешнюю логику (которая пользуется pgFile)
или вообще сделать два "конструктора" pgFile, один для создания pgFile из файловой системы (из dir_check_file()), второй из бекапа (get_backup_filelist()).
В любом случае в функции get_backup_filelist() смесь логики чтения файла и заполнения внутренних полей pgFile мне кажется очень некрасивой, хотелось бы побольше убрать внутрь src/dir.c.
Но это моё имхо, что думаешь?