Skip to content

RemoteOperations::exec_command explicitly transfers LANG, LANGUAGE and LC_* envvars to the server side #187

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

Conversation

dmitry-lipetsk
Copy link
Collaborator

@dmitry-lipetsk dmitry-lipetsk commented Feb 20, 2025

It should help resolve a problem with replacing the LANG variable by ssh-server.

History.

On our internal tests we got a problem on the Debian 11 and PostgresPro STD-13.

One test returned the error of initdb:

initdb: error: collations with different collate and ctype values ("en_US.UTF-8" and "C.UTF-8" accordingly) are not supported by ICU

  • A test runner sets variable LANG="C"
  • Python sets variable LC_CTYPE="C.UTF-8"
  • A test calls initdb through command "ssh test@localhost inidb -D ...."
  • SSH-server replaces LANG with value "en_US.UTF-8" (from etc/default/locale)
  • initdb calculates a collate through this value of LANG variable and gets "en_US.UTF-8"

So we have that:

  • ctype is "C.UTF-8"
  • collate is "en_US.UTF-8"

ICU on the Debuan-11 (uconv v2.1 ICU 67.1) does not support this combination and initdb returns the error.

This patch generates a new command line for ssh:

ssh test@localhost "LANG=\"...\";LC_xxx=\"...\";<command>"

It resolves this problem with initdb and should help resolve other problems with execution of commands through SSH.

- Exact enumeration of supported 'cmd' types
- Refactoring
…d LC_* envvars to the server side

It should help resolve a problem with replacing a LANG variable by ssh-server.

History.

On our internal tests we got a problem on the Debian 11 and PostgresPro STD-13.

One test returned the error from initdb:

initdb: error: collations with different collate and ctype values ("en_US.UTF-8" and "C.UTF-8" accordingly) are not supported by ICU

- TestRunner set variable LANG="C"
- Python set variable LC_CTYPE="C.UTF-8"
- Test call inidb through command "ssh test@localhost inidb -D ...."
- SSH-server replaces LANG with value "en_US.UTF-8" (from etc/default/locale)
- initdb calculate collate through this value of LANG variable and get en_US.UTF-8

So we have that:
- ctype is C.UTF-8
- collate is en_US.UTF-8

ICU on the Debuan-11 (uconv v2.1  ICU 67.1) does not suppot this combination and inidb rturns the error.

This patch generates a new command line for ssh:

ssh test@localhost "LANG=\"...\";LC_xxx=\"...\";<command>"

It resolves this problem with initdb and should help resolve other problems with execution of command through SSH.

Amen.
@dmitry-lipetsk
Copy link
Collaborator Author

Patch was tested on the following OS:

  • redos-8
  • debian-11
  • ubuntu-24.04

New tests:
 - test_init__LANG_С
 - test_init__unk_LANG_and_LC_CTYPE
@@ -510,6 +515,42 @@ def db_connect(self, dbname, user, password=None, host="localhost", port=5432):
)
return conn

def _make_exec_env_list() -> list[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

тут нужно self, либо добавить @staticmethod

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

self тут не нужен. Это статический метод.

Хорошо, я добавлю этот декоратор.

Я там в других местах тоже статические методы определял...

Тогда я пока только в новом коде этого PR его добавлю, а потом, отдельным коммитом, добавим его во все остальные случаи.

PS. Я этот питон на ходу изучаю, поэтому многие вещи использую неправильно ))))


sm_envs_for_exec_cmd = ["LANG", "LANGUAGE"]

def _does_put_envvar_into_exec_cmd(name: str) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

тут нужно self, либо добавить @staticmethod

return True
return False

def _quote_envvar(value) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

тут нужно self, либо добавить @staticmethod

def _quote_envvar(value) -> str:
assert type(value) == str # noqa: E721
result = "\""
for ch in value:
Copy link
Contributor

Choose a reason for hiding this comment

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

Тут лучше append использовать https://www.geeksforgeeks.org/difference-between-and-append-in-python/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Виктория, что-то я не догоняю. Куда тут append использовать?

result - это str. У него нет метода append, но есть поддержка оператора +=, который по смыслу тот же append. Этот += тут и используется.

Copy link
Contributor

@demonolock demonolock Feb 21, 2025

Choose a reason for hiding this comment

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

типо так


       result = ['"']
        for ch in value:
            if ch == '"':
                result.append('\\"')
            elif ch == '\\':
                result.append('\\\\')
            else:
                result.append(ch)
        result.append('"')
        return ''.join(result)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ага. Не. Не надо нам такой "оптимизации" )

Во первых у нас тут строка. И её элементы могут хранится непрерывно, ну или по крайней мере питон там может как-то оптимизировать это дело. А если будет массив строк - то однозначно каждый элемент массива будет хранится отдельно. Плюс место под сам массив.

А во вторых у нас тут нет оператора сложения. Используется оператор +=.

С append можно заморочится выше - там я три массива складываю через +.

Но честно - если бы эти бестолочи (я про авторов питона) позволяли писать append(list1).append(list2), я бы его заюзал. А так - в топку.


25 лет назад на .NET считалось дурным тоном складывать строки. Юзайте, говорят, StringBuilder. Но потом все дружно на него забили ))

Let's test bad data with '\' and '"' symbols.
…ard]

The following methods of RemoteOperations were corrected:
 - _make_exec_env_list
 - _does_put_envvar_into_exec_cmd
 - _quote_envvar
@demonolock demonolock merged commit e44aa98 into postgrespro:master Feb 21, 2025
1 check passed
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.

2 participants